codeant-ai-for-open-source[bot] commented on code in PR #33247:
URL: https://github.com/apache/superset/pull/33247#discussion_r3407730759
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/controlPanel.ts:
##########
@@ -352,15 +354,56 @@ const config: ControlPanelConfig = {
},
],
[
+ {
+ name: 'point_radius',
+ config: {
+ type: 'SelectControl',
+ freeForm: true,
+ label: t('Point Radius'),
+ description: t(
+ 'The radius of point features, in the units specified below. '
+
+ 'The final rendered size is this value multiplied by Point
Radius Scale.',
+ ),
+ validators: [validateInteger],
+ default: 10,
+ choices: formatSelectOptions([1, 5, 10, 20, 50, 100]),
+ renderTrigger: true,
+ },
+ },
{
name: 'point_radius_scale',
config: {
type: 'SelectControl',
freeForm: true,
label: t('Point Radius Scale'),
- validators: [legacyValidateInteger],
- default: null,
- choices: formatSelectOptions([0, 100, 200, 300, 500]),
+ description: t(
+ 'A multiplier applied to the point radius. ' +
+ 'Use this to uniformly scale all points.',
+ ),
+ validators: [validateNumber],
+ default: 1,
+ choices: formatSelectOptions([0.1, 0.5, 1, 2, 5, 10]),
+ renderTrigger: true,
+ },
+ },
+ ],
+ [
+ {
+ name: 'point_radius_units',
+ config: {
+ type: 'SelectControl',
+ label: t('Point Radius Units'),
+ description: t(
+ 'The unit for point radius. Use "pixels" for consistent ' +
+ 'screen-space sizing regardless of zoom level.',
+ ),
+ default: 'pixels',
Review Comment:
**Suggestion:** These defaults break backward compatibility for existing
dashboard slices: `applyDefaultFormData` auto-fills missing controls with panel
defaults, so legacy GeoJSON charts that never had these fields will be hydrated
as `point_radius=10` and `point_radius_units='pixels'`, changing rendering from
the prior implicit deck.gl behavior (`1` + `meters`). To preserve existing
charts, avoid defaulting new controls at control-panel level for legacy form
data (e.g., use undefined/null defaults and apply new defaults only for newly
created charts or via explicit migration/version gating). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Existing Geojson dashboard charts render with larger point radii.
- ❌ Existing Geojson charts switch from meter to pixel sizing.
- ⚠️ Visual regressions across dashboards after upgrading with this PR.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Take an existing deck.gl Geojson chart whose persisted `slice.form_data`
has no
`point_radius` or `point_radius_units` keys (this is the legacy state before
these
controls existed and is what `charts.forEach` reads in
`src/dashboard/actions/hydrate.ts:157-15`).
2. Load a dashboard containing that slice; during hydration in
`src/dashboard/actions/hydrate.ts:157-21`, the code builds `formData` and
calls
`applyDefaultFormData(formData)` to produce the chart's `form_data` for
`chartQueries[key]`.
3. Inside `applyDefaultFormData` (`src/explore/store.ts:127-158`),
`getAllControlsState`
(`src/explore/controlUtils/getControlState.ts:168-198`) constructs control
state for the
Geojson viz; because `cleanedFormData.point_radius` and
`cleanedFormData.point_radius_units` are `undefined`,
`getControlStateFromControlConfig`
and `applyMapStateToPropsToControl` (`getControlState.ts:102-128`) apply the
control-panel
defaults from `Geojson/controlPanel.ts:358-372` and `392-400`, setting
`control.value` to
`10` for `point_radius` and `'pixels'` for `point_radius_units`.
4. The merged `form_data` returned by `applyDefaultFormData` now includes
`point_radius:
10` and `point_radius_units: 'pixels'`, and this is passed to the Geojson
plugin; in
`Geojson.tsx:72-76`, `getPointRadius: fd.point_radius ?? 1` and
`pointRadiusUnits:
fd.point_radius_units ?? 'meters'` therefore use `10` and `'pixels'` instead
of falling
back to deck.gl defaults (`1`, `'meters'`), so when the dashboard finishes
loading the
map's point circles are noticeably larger and now screen-space sized,
changing the
appearance of all legacy Geojson charts without any user action.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ef2262e6777f49549e7f1abe60354fdd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ef2262e6777f49549e7f1abe60354fdd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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/controlPanel.ts
**Line:** 368:400
**Comment:**
*Logic Error: These defaults break backward compatibility for existing
dashboard slices: `applyDefaultFormData` auto-fills missing controls with panel
defaults, so legacy GeoJSON charts that never had these fields will be hydrated
as `point_radius=10` and `point_radius_units='pixels'`, changing rendering from
the prior implicit deck.gl behavior (`1` + `meters`). To preserve existing
charts, avoid defaulting new controls at control-panel level for legacy form
data (e.g., use undefined/null defaults and apply new defaults only for newly
created charts or via explicit migration/version gating).
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F33247&comment_hash=313769b827b82c50a13bd13d6b3c8432ed5092473e17f6aa16adf933901ffac4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F33247&comment_hash=313769b827b82c50a13bd13d6b3c8432ed5092473e17f6aa16adf933901ffac4&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]
