codeant-ai-for-open-source[bot] commented on code in PR #37248:
URL: https://github.com/apache/superset/pull/37248#discussion_r2704568386
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Path/controlPanel.ts:
##########
@@ -75,7 +75,7 @@ const config: ControlPanelConfig = {
config: {
type: 'SelectControl',
label: t('Line width unit'),
- default: 'pixels',
+ default: 'meters',
Review Comment:
**Suggestion:** Backwards-compatibility / regression: changing the control's
default from the previous value to 'meters' will silently change the appearance
of all existing saved charts that did not have an explicit `line_width_unit`
set. This is a breaking behavioral change and should either be accompanied by a
migration that updates existing saved charts or avoided (keep the previous
default) so dashboards' visuals don't change unexpectedly. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Existing Path charts change visual appearance unexpectedly.
- ⚠️ Dashboards containing Path charts show visual regressions.
- ⚠️ Users must manually update charts or re-save with explicit unit.
```
</details>
```suggestion
// NOTE: Changing this default from 'pixels' to 'meters' is a
breaking change
// for existing saved charts. Keep the previous default here
(pixels) and
// perform an explicit migration or surface an opt-in to flip
existing charts.
default: 'pixels',
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Identify an existing saved Path chart created before this PR whose stored
form_data
does NOT include a `line_width_unit` key (saved chart metadata lives in the
Superset
backend slices/dashboard metadata; this is the typical case when a control
was previously
not exposed or left unset).
2. Upgrade the frontend to include the PR changes so that
`superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Path/controlPanel.ts`
is
used (see the control definition at lines 74-78 where `name:
'line_width_unit'` and the
default are defined).
3. Open the saved chart in the Explore/Edit UI; the frontend builds controls
using the
ControlPanelConfig from `controlPanel.ts` (type imported from
'@superset-ui/chart-controls' at top of the file). Because the saved
form_data lacks
`line_width_unit`, the UI/code will fall back to the control's default
defined at
controlPanel.ts:78 (`default: 'meters'`).
4. Observe the chart rendering in the dashboard/Explore view: line widths
for Path layer
now interpret widths in meters instead of pixels, producing visually
different
(thicker/thinner) paths compared to the pre-PR rendering. This demonstrates
the regression
for saved charts that did not persist an explicit `line_width_unit`.
```
</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/Path/controlPanel.ts
**Line:** 78:78
**Comment:**
*Possible Bug: Backwards-compatibility / regression: changing the
control's default from the previous value to 'meters' will silently change the
appearance of all existing saved charts that did not have an explicit
`line_width_unit` set. This is a breaking behavioral change and should either
be accompanied by a migration that updates existing saved charts or avoided
(keep the previous default) so dashboards' visuals don't change unexpectedly.
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]
