fix(maps): preserve OSM map renderer choices#40804
Open
richardfogaca wants to merge 8 commits into
Open
Conversation
github-actions
Bot
added
risk:db-migration
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
richardfogaca
requested review from
betodealmeida,
eschutho,
michael-s-molina and
sadpandajoe
as code owners
dosubot
Bot
added
change:frontend
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes map chart compatibility and saved-state preservation during the MapLibre migration by ensuring only true mapbox:// styles remain Mapbox-backed, while legacy non-Mapbox style strings (including OSM and raster tile templates) stay on the MapLibre path and render with the intended styles across affected chart plugins.
Changes:
- Adjusts the
ce6bd21901abmigration behavior to classify Mapbox only formapbox://styles and to preserve legacy non-Mapbox styles viamaplibre_style. - Introduces shared frontend map utilities (
mapStyles) for Mapbox-key detection, renderer option gating, and raster tile template style resolution (incl. OSM choice), and reuses them across deck.gl + point-cluster controls/rendering. - Propagates provider/style/key props into GeoJSON and Polygon rendering, adds/updates focused Jest + migration test coverage, and documents upgrade behavior.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| UPDATING.md | Documents the migration behavior, OSM attribution expectations, and Mapbox no-key behavior. |
| tests/integration_tests/migrations/ce6bd21901ab_migrate_deckgl_and_mapbox__test.py | Expands migration regression coverage for style classification and preservation rules. |
| superset/migrations/versions/2026-03-02_00-00_ce6bd21901ab_migrate_deckgl_and_mapbox.py | Updates migration logic to preserve non-Mapbox styles on the MapLibre path. |
| superset-frontend/plugins/preset-chart-deckgl/src/utils/mapbox.ts | Switches Mapbox key access to shared bootstrap helpers. |
| superset-frontend/plugins/preset-chart-deckgl/src/utils/mapbox.test.ts | Adds unit coverage for deck.gl Mapbox bootstrap key helpers. |
| superset-frontend/plugins/preset-chart-deckgl/src/utilities/Shared_DeckGL.tsx | Reuses shared renderer/style helpers; adds OSM style choice and key-gated renderer options. |
| superset-frontend/plugins/preset-chart-deckgl/src/utilities/Shared_DeckGL.test.ts | Adds tests for OSM choice availability and Mapbox option gating behavior. |
| superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/Polygon.tsx | Propagates renderer/style/key props into the shared DeckGL container. |
| superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/Polygon.test.tsx | Adds assertions for renderer/style/key propagation to the container. |
| superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/Geojson.tsx | Makes getPoints more defensive and propagates renderer/style/key props into the container. |
| superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/Geojson.test.tsx | Adds component + helper tests, including propagation assertions. |
| superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/Geojson.test.ts | Removes the older non-component GeoJSON test file (replaced by .tsx suite). |
| superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/controlPanel.ts | Expands the Map control section for visibility of renderer controls. |
| superset-frontend/plugins/preset-chart-deckgl/src/DeckGLContainer.tsx | Resolves MapLibre raster tile templates into MapLibre style objects and preserves Mapbox no-key message path. |
| superset-frontend/plugins/preset-chart-deckgl/src/DeckGLContainer.test.tsx | Adds unit coverage for style resolution + Mapbox no-key behavior + ref tooltip behavior. |
| superset-frontend/plugins/plugin-chart-point-cluster-map/test/transformProps.test.ts | Adds assertions that provider/style selection is reflected in transform props. |
| superset-frontend/plugins/plugin-chart-point-cluster-map/test/MapLibre.test.tsx | Adds tests for raster-tile resolution and Mapbox no-key behavior. |
| superset-frontend/plugins/plugin-chart-point-cluster-map/test/controlPanel.test.ts | Adds tests for OSM style choice and Mapbox key-gated renderer options. |
| superset-frontend/plugins/plugin-chart-point-cluster-map/src/utils/mapControls.ts | Centralizes point-cluster renderer options + MapLibre style choices with shared utilities. |
| superset-frontend/plugins/plugin-chart-point-cluster-map/src/utils/mapbox.ts | Reuses shared bootstrap Mapbox key helpers. |
| superset-frontend/plugins/plugin-chart-point-cluster-map/src/MapLibre.tsx | Resolves raster tile templates for MapLibre style inputs and preserves Mapbox no-key path. |
| superset-frontend/plugins/plugin-chart-point-cluster-map/src/controlPanel.ts | Uses shared renderer option gating and shared MapLibre style choice list (incl. OSM). |
| superset-frontend/packages/superset-ui-core/src/utils/mapStyles.ts | Adds shared map renderer/style/key/raster-template utilities used by multiple plugins. |
| superset-frontend/packages/superset-ui-core/src/utils/mapStyles.test.ts | Adds unit tests for shared map utilities. |
| superset-frontend/packages/superset-ui-core/src/utils/index.ts | Exports new shared mapStyles utilities. |
| superset-frontend/package-lock.json | Adjusts deck.gl Mapbox dependency range (^ → ~). |
| .compozy/tasks/preserve-osm-custom-map-charts/task_03.md | Task tracking artifact for point-cluster alignment work. |
| .compozy/tasks/preserve-osm-custom-map-charts/task_04.md | Task tracking artifact for GeoJSON/Polygon propagation work. |
| .compozy/tasks/preserve-osm-custom-map-charts/task_05.md | Task tracking artifact for upgrade/support documentation work. |
| .compozy/tasks/preserve-osm-custom-map-charts/_tasks.md | Task list tracking artifact for the overall effort. |
Files not reviewed (1)
- superset-frontend/package-lock.json: Language not supported
Comment on lines
+19
to
+21
|
|
||
| export type MapProvider = 'maplibre' | 'mapbox'; | ||
|
|
Comment on lines
+60
to
+64
| export const OSM_TILE_STYLE_CHOICE: MapStyleChoice = { | ||
| value: OSM_TILE_STYLE_URL, | ||
| label: 'Streets (OSM)', | ||
| attribution: OSM_TILE_ATTRIBUTION, | ||
| }; |
Comment on lines
+66
to
+69
| export const MAPLIBRE_RENDERER_OPTION: MapRendererOption = { | ||
| value: 'maplibre', | ||
| label: 'MapLibre (open-source)', | ||
| }; |
Comment on lines
+70
to
+73
| export const MAPBOX_RENDERER_OPTION: MapRendererOption = { | ||
| value: 'mapbox', | ||
| label: 'Mapbox (API key required)', | ||
| }; |
Comment on lines
+74
to
+78
| export const DISABLED_MAPBOX_RENDERER_OPTION: MapRendererOption = { | ||
| ...MAPBOX_RENDERER_OPTION, | ||
| label: 'Mapbox (MAPBOX_API_KEY required)', | ||
| disabled: true, | ||
| }; |
Comment on lines
+148
to
+169
| export function buildRasterTileMapStyle(value: string): RasterTileMapStyle { | ||
| return { | ||
| version: 8, | ||
| sources: { | ||
| [RASTER_SOURCE_ID]: { | ||
| type: 'raster', | ||
| tiles: [unwrapTileProtocol(value)], | ||
| tileSize: 256, | ||
| attribution: OSM_TILE_ATTRIBUTION, | ||
| }, | ||
| }, | ||
| layers: [ | ||
| { | ||
| id: RASTER_LAYER_ID, | ||
| type: 'raster', | ||
| source: RASTER_SOURCE_ID, | ||
| minzoom: 0, | ||
| maxzoom: 22, | ||
| }, | ||
| ], | ||
| }; | ||
| } |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+91
to
+99
| const isDeckGLTileChoices = (value: unknown): value is DeckGLTileChoice[] => | ||
| Array.isArray(value) && | ||
| value.every( | ||
| choice => | ||
| Array.isArray(choice) && | ||
| choice.length >= 2 && | ||
| typeof choice[0] === 'string' && | ||
| typeof choice[1] === 'string', | ||
| ); |
Contributor
There was a problem hiding this comment.
Suggestion: The tile-choice validator accepts an empty array as valid, so a deployment with DECKGL_BASE_MAP=[] will set deckglTiles to [] and later crash when maplibreStyle reads getDeckGLTiles()[0][0]. Tighten the validator to require at least one entry (and ideally exactly two string elements per choice) before accepting overrides, otherwise fall back to DEFAULT_DECKGL_TILES. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ DeckGL Explore controls crash when DECKGL_BASE_MAP is empty.
- ⚠️ All DeckGL map layers become unusable under misconfiguration.
- ⚠️ Admins cannot rely on empty list as override.Steps of Reproduction ✅
1. Configure Superset with an empty DeckGL base map list by setting `DECKGL_BASE_MAP = []`
in `superset_config.py` as described in
`docs/admin_docs/configuration/map-tiles.mdx:13-34`. When the server builds bootstrap data
in `superset/views/base.py:16`, it copies this configuration into the `"deckgl_tiles"`
field of the `bootstrap_data` dict.
2. Start the Superset web server so that the frontend receives `deckgl_tiles: []` in the
bootstrap payload embedded in the HTML, which is then read by
`getBootstrapDataFromDocument()` inside
`superset-frontend/plugins/preset-chart-deckgl/src/utilities/Shared_DeckGL.tsx` (see the
`getDeckGLTiles` function around lines 22–32 in the Read output).
3. In the browser, open Explore for any DeckGL-based chart such as the Hex layer
(`superset-frontend/plugins/preset-chart-deckgl/src/layers/Hex/controlPanel.ts:25-47`),
which imports `maplibreStyle` from `../../utilities/Shared_DeckGL` and includes it in the
`"Map"` control section.
4. When the control panel renders, `maplibreStyle.config` in `Shared_DeckGL.tsx` (shown in
the Read output lines 21–31) evaluates `choices: getDeckGLTiles()` and `default:
getDeckGLTiles()[0][0]`. Because `isDeckGLTileChoices` (PR lines 91–99) treats `[]` as
valid, `getDeckGLTiles()` returns `[]`, so `getDeckGLTiles()[0]` is `undefined` and `[0]`
access throws a runtime error in the browser, breaking the Explore UI for DeckGL charts
under this configuration.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/preset-chart-deckgl/src/utilities/Shared_DeckGL.tsx
**Line:** 91:99
**Comment:**
*Incorrect Condition Logic: The tile-choice validator accepts an empty array as valid, so a deployment with `DECKGL_BASE_MAP=[]` will set `deckglTiles` to `[]` and later crash when `maplibreStyle` reads `getDeckGLTiles()[0][0]`. Tighten the validator to require at least one entry (and ideally exactly two string elements per choice) before accepting overrides, otherwise fall back to `DEFAULT_DECKGL_TILES`.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| type: 'raster', | ||
| tiles: [unwrapTileProtocol(value)], | ||
| tileSize: 256, | ||
| attribution: OSM_TILE_ATTRIBUTION, |
Contributor
There was a problem hiding this comment.
Suggestion: buildRasterTileMapStyle hardcodes OpenStreetMap attribution for every raster tile template, including user-provided free-form URLs. This will display incorrect attribution text for non-OSM providers and can violate tile-provider attribution requirements. Pass attribution as an input (or only apply OSM attribution when the URL is the known OSM template) instead of forcing OSM_TILE_ATTRIBUTION for all raster styles. [logic error]
Severity Level: Major ⚠️
- ❌ Non-OSM raster tiles show incorrect OpenStreetMap attribution.
- ⚠️ Violates custom tile provider attribution requirements.Steps of Reproduction ✅
1. Create or migrate a Deck GL-based map chart so that its form data uses the MapLibre
renderer with a raster tile template, for example by saving chart params where
`maplibre_style` is a `tile://` URL like
`tile://https://tiles.example.com/{z}/{x}/{y}.png` (params structure and `maplibre_style`
field are exercised in
`tests/integration_tests/migrations/ce6bd21901ab_migrate_deckgl_and_mapbox__test.py:101-117`).
2. When the chart is rendered, the Deck GL factory at
`superset-frontend/plugins/preset-chart-deckgl/src/factory.tsx:194-201` passes
`formData.maplibre_style` into `DeckGLContainerStyledWrapper` as the `mapStyle` prop and
sets `mapProvider` to `'maplibre'`.
3. Inside `DeckGLContainer` at
`superset-frontend/plugins/preset-chart-deckgl/src/DeckGLContainer.tsx:11-15`, because
`mapProvider` is `'maplibre'`, it computes `mapStyle` by calling
`resolveMapStyle(props.mapStyle, DEFAULT_MAP_STYLE)`, where `props.mapStyle` is the
user-supplied tile template URL.
4. `resolveMapStyle` in
`superset-frontend/packages/superset-ui-core/src/utils/mapStyles.ts:52-60` calls
`isRasterTileTemplate(value)`; `isRasterTileTemplate` only checks for `{z}`, `{x}`, `{y}`
placeholders after unwrapping the `tile://` protocol (lines 19-27), so any
`tile://http.../{z}/{x}/{y}.png` URL (including non-OSM providers) is treated as a raster
template.
5. For such a raster template, `resolveMapStyle` calls `buildRasterTileMapStyle(value)` at
`mapStyles.ts:29-49`, which constructs a MapLibre style object whose source attribution is
hardcoded to `OSM_TILE_ATTRIBUTION` (`'© OpenStreetMap contributors'`) at
`mapStyles.ts:37`, regardless of the actual tile host.
6. The resulting style object is passed into the MapLibre map component in
`DeckGLContainer` (`DeckGLContainer.tsx:50-67`), so the rendered map displays an
OpenStreetMap attribution string even when the tiles come from a non-OSM provider, leading
to incorrect and potentially non-compliant attribution for user-configured tile servers.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/utils/mapStyles.ts
**Line:** 156:156
**Comment:**
*Logic Error: `buildRasterTileMapStyle` hardcodes OpenStreetMap attribution for every raster tile template, including user-provided free-form URLs. This will display incorrect attribution text for non-OSM providers and can violate tile-provider attribution requirements. Pass attribution as an input (or only apply OSM attribution when the URL is the known OSM template) instead of forcing `OSM_TILE_ATTRIBUTION` for all raster styles.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
Code Review Agent Run #a4f99fActionable Suggestions - 0Additional Suggestions - 3
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Fixes map chart compatibility around the MapLibre migration:
mapbox://styles Mapbox-backed, so deployments withoutMAPBOX_API_KEYcontinue to show the existing missing-key signal rather than silently changing providers.Streets (OSM)MapLibre-compatible style with OpenStreetMap tile attribution and shares the renderer/style helpers across Deck GL and point-cluster controls.UPDATING.md.What Changed
mapbox://styles choose the Mapbox renderer; non-Mapbox legacy style strings are copied tomaplibre_stylewhen that value is absent.@superset-ui/coreutilities and are reused by affected map plugins.UPDATING.mddocuments non-Mapbox preservation, true Mapbox no-key behavior, and OSM attribution expectations.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - no provider-visible media is attached in this draft. The affected UI behavior was exercised in a local Superset runtime: no-key Deck GL and point-cluster Explore controls showed
Streets (OSM)as available with Mapbox disabled, saved true Mapbox charts showed theMAPBOX_API_KEYrequired message, and GeoJSON/Polygon charts rendered MapLibre/OSM maps with attribution.TESTING INSTRUCTIONS
Repeatable checks:
Validation
Live browser validation in a local Superset runtime with
MAPBOX_API_KEYunset:Streets (OSM)remains selectable and Mapbox is disabled for new selection.MAPBOX_API_KEYrequired message is preserved.Reviewer Focus
map_rendererand existingmaplibre_stylevalues should not be overwritten, and truemapbox://styles should remain Mapbox-backed.ADDITIONAL INFORMATION
Migration notes: this updates the existing MapLibre migration before an application release tag contains the upstream migration. The change only rewrites chart metadata JSON during the migration path, preserves existing renderer choices and existing
maplibre_style, and keeps downgrade behavior scoped to removing the migration-added renderer marker. Runtime impact is bounded to the existing paginated chart-slice migration.