korbit-ai[bot] commented on code in PR #32867:
URL: https://github.com/apache/superset/pull/32867#discussion_r2014639507
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utilities/Shared_DeckGL.jsx:
##########
@@ -29,6 +29,20 @@ import {
import { D3_FORMAT_OPTIONS, sharedControls } from
'@superset-ui/chart-controls';
import { columnChoices, PRIMARY_COLOR } from './controls';
+
+export const DEFAULT_DECKGL_TILES = [
+ ['mapbox://styles/mapbox/streets-v9', t('Streets')],
+ ['mapbox://styles/mapbox/dark-v9', t('Dark')],
+ ['mapbox://styles/mapbox/light-v9', t('Light')],
+ ['tile://https://c.tile.openstreetmap.org/{z}/{x}/{y}.png',
t('OpenStreetMap')],
+];
+
+const appContainer = document.getElementById('app');
+const { common } = JSON.parse(
+ appContainer?.getAttribute('data-bootstrap') || '{}',
+);
+const deckgl_tiles = common?.deckgl_tiles ?? DEFAULT_DECKGL_TILES;
Review Comment:
### Eager DOM Access and JSON Parse <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
DOM querying and JSON parsing operations are performed during module
initialization, which executes every time the module is imported.
###### Why this matters
This can impact the module's load time and overall application performance,
especially if the module is imported frequently or in performance-critical
paths.
###### Suggested change ∙ *Feature Preview*
Move the DOM querying and JSON parsing into a lazy initialization function
that's called only when needed:
```jsx
let deckgl_tiles;
const getDeckGLTiles = () => {
if (!deckgl_tiles) {
const appContainer = document.getElementById('app');
const { common } = JSON.parse(
appContainer?.getAttribute('data-bootstrap') || '{}',
);
deckgl_tiles = common?.deckgl_tiles ?? DEFAULT_DECKGL_TILES;
}
return deckgl_tiles;
};
```
Then update the mapboxStyle configuration to use this function:
```jsx
choices: getDeckGLTiles(),
default: getDeckGLTiles()[0][0],
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/50560187-0b2f-46ba-a567-1a99735210af/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/50560187-0b2f-46ba-a567-1a99735210af?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/50560187-0b2f-46ba-a567-1a99735210af?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/50560187-0b2f-46ba-a567-1a99735210af?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/50560187-0b2f-46ba-a567-1a99735210af)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0e578144-f3b5-4186-b459-9b3f0b58dbeb -->
[](0e578144-f3b5-4186-b459-9b3f0b58dbeb)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/DeckGLContainer.tsx:
##########
@@ -99,8 +101,14 @@ export const DeckGLContainer = memo(
) as Layer[];
}
+ if (props.mapStyle?.startsWith(TILE_LAYER_PREFIX)) {
+ props.layers.unshift(
+ buildTileLayer(props.mapStyle.replace(TILE_LAYER_PREFIX, '')),
+ );
+ }
Review Comment:
### Direct Props Mutation in Layers Function <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The layers function modifies the props.layers array directly using
unshift(), which mutates the original array and can cause unexpected behavior
in React.
###### Why this matters
Mutating props directly can lead to rendering issues and make component
behavior unpredictable, especially if the layers array is used elsewhere in the
application.
###### Suggested change ∙ *Feature Preview*
Create a new array instead of modifying the existing one:
```typescript
if (props.mapStyle?.startsWith(TILE_LAYER_PREFIX)) {
return [
buildTileLayer(props.mapStyle.replace(TILE_LAYER_PREFIX, '')),
...props.layers,
];
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11166048-cfdd-4c8a-a865-5f53c17303aa/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11166048-cfdd-4c8a-a865-5f53c17303aa?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11166048-cfdd-4c8a-a865-5f53c17303aa?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11166048-cfdd-4c8a-a865-5f53c17303aa?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11166048-cfdd-4c8a-a865-5f53c17303aa)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:04418026-ecaf-49bb-bdb2-523eb0c5d5fc -->
[](04418026-ecaf-49bb-bdb2-523eb0c5d5fc)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/DeckGLContainer.tsx:
##########
@@ -115,11 +123,28 @@
viewState={viewState}
onViewStateChange={onViewStateChange}
>
- <StaticMap
- preserveDrawingBuffer
- mapStyle={props.mapStyle || 'light'}
- mapboxApiAccessToken={props.mapboxApiAccessToken}
- />
+ {props.mapStyle?.startsWith(MAPBOX_LAYER_PREFIX) && (
+ <StaticMap
+ preserveDrawingBuffer
+ mapStyle={props.mapStyle || 'light'}
+ mapboxApiAccessToken={props.mapboxApiAccessToken}
+ />
+ )}
Review Comment:
### Ineffective Fallback MapStyle <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The fallback 'light' mapStyle will never be used because the conditional
check requires mapStyle to exist for the StaticMap to render.
###### Why this matters
If mapStyle is undefined, the StaticMap component won't render at all,
making the fallback style ineffective.
###### Suggested change ∙ *Feature Preview*
Move the fallback to the conditional check:
```typescript
{(props.mapStyle?.startsWith(MAPBOX_LAYER_PREFIX) || props.mapStyle ===
undefined) && (
<StaticMap
preserveDrawingBuffer
mapStyle={props.mapStyle || 'light'}
mapboxApiAccessToken={props.mapboxApiAccessToken}
/>
)}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4594b5ed-2eaf-4bd5-9494-0284f6920c4b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4594b5ed-2eaf-4bd5-9494-0284f6920c4b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4594b5ed-2eaf-4bd5-9494-0284f6920c4b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4594b5ed-2eaf-4bd5-9494-0284f6920c4b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4594b5ed-2eaf-4bd5-9494-0284f6920c4b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:3c5617ca-e7fe-42d9-8b13-559def4b9f11 -->
[](3c5617ca-e7fe-42d9-8b13-559def4b9f11)
##########
superset-frontend/packages/superset-ui-core/src/validator/validateMapboxStylesUrl.ts:
##########
@@ -21,16 +21,20 @@ import { t } from '../translation';
/**
* Validate a [Mapbox styles
URL](https://docs.mapbox.com/help/glossary/style-url/)
+ * or a title server URL.
* @param v
*/
export default function validateMapboxStylesUrl(v: unknown) {
if (
typeof v === 'string' &&
v.trim().length > 0 &&
- v.trim().startsWith('mapbox://styles/')
+ (v.trim().startsWith('mapbox://styles/') ||
+ v.trim().startsWith('tile://http'))
) {
return false;
}
Review Comment:
### Counter-intuitive validation return values <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The validation function returns false for valid URLs and a string message
for invalid URLs, which is counter-intuitive and could lead to confusion.
###### Why this matters
This inverted boolean return value could cause bugs in the application where
valid URLs are treated as invalid and vice versa. Most validation functions
return true for valid input and false or an error message for invalid input.
###### Suggested change ∙ *Feature Preview*
Invert the return values to match common validation patterns:
```typescript
export default function validateMapboxStylesUrl(v: unknown) {
if (
typeof v === 'string' &&
v.trim().length > 0 &&
(v.trim().startsWith('mapbox://styles/') ||
v.trim().startsWith('tile://http'))
) {
return true;
}
return t(
'is expected to be a Mapbox URL (eg. mapbox://styles/...) or a tile
server URL (eg. tile://http...)',
);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9c956e4-ba4c-4aa3-b041-625beaaa5f50/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9c956e4-ba4c-4aa3-b041-625beaaa5f50?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9c956e4-ba4c-4aa3-b041-625beaaa5f50?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9c956e4-ba4c-4aa3-b041-625beaaa5f50?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b9c956e4-ba4c-4aa3-b041-625beaaa5f50)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:53391593-ef0d-40b9-84fa-8f54a892f0c3 -->
[](53391593-ef0d-40b9-84fa-8f54a892f0c3)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/DeckGLContainer.tsx:
##########
@@ -115,11 +123,28 @@
viewState={viewState}
onViewStateChange={onViewStateChange}
>
- <StaticMap
- preserveDrawingBuffer
- mapStyle={props.mapStyle || 'light'}
- mapboxApiAccessToken={props.mapboxApiAccessToken}
- />
+ {props.mapStyle?.startsWith(MAPBOX_LAYER_PREFIX) && (
+ <StaticMap
+ preserveDrawingBuffer
+ mapStyle={props.mapStyle || 'light'}
+ mapboxApiAccessToken={props.mapboxApiAccessToken}
+ />
+ )}
+ {props.mapStyle?.indexOf('openstreetmap') !== -1 && (
Review Comment:
### Hardcoded map style string literal with unclear comparison
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using string literal 'openstreetmap' and indexOf for type checking is less
readable than using a constant and includes()
###### Why this matters
String literals scattered in the code make it harder to maintain and
understand the valid map styles. The indexOf method is also less intuitive for
checking string containment.
###### Suggested change ∙ *Feature Preview*
```typescript
const OSM_LAYER_PREFIX = 'openstreetmap';
// ...
props.mapStyle?.includes(OSM_LAYER_PREFIX)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/90952bed-123b-4d48-a2d7-ee0248405374/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/90952bed-123b-4d48-a2d7-ee0248405374?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/90952bed-123b-4d48-a2d7-ee0248405374?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/90952bed-123b-4d48-a2d7-ee0248405374?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/90952bed-123b-4d48-a2d7-ee0248405374)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:40503d90-b545-4a6b-8bec-80d9ff962ba8 -->
[](40503d90-b545-4a6b-8bec-80d9ff962ba8)
--
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]
