EnxDev commented on PR #41859:
URL: https://github.com/apache/superset/pull/41859#issuecomment-4961779217
## EnxDev's Review Agent โ apache/superset#41859 ยท HEAD 564bb31
**request changes** โ the regression this PR set out to prevent still exists
for the regular Bar chart, and CI (`lint-frontend`, `pre-commit`) is red.
Feature/fix classification. Verified against the diff and `master` in a
local clone. Reviewed the bot threads: the `codeant` "Critical" comment on
`Timeseries/transformProps.ts:317` is the key one โ it was marked "fixed" but
the fix landed only in the Mixed path.
### ๐ด Functional
- **`src/Timeseries/transformProps.ts:317`** ยท _High_ โ The
overflow-protection fix was ported to `MixedTimeseries/transformProps.ts` but
**not** to the regular Bar chart. `getAxisType(stack, xAxisForceCategorical,
xAxisDataType)` here has no runtime fallback, and `xAxisForceCategorical`
resolves from `{...DEFAULT_FORM_DATA, ...formData}` where
`DEFAULT_FORM_DATA.xAxisForceCategorical === false`
(`Timeseries/constants.ts:81`). So an **existing** unstacked bar chart on a
numeric x-axis (e.g. `year`) with the flag stored `false` (or absent) now
renders `AxisType.Value` on dashboards โ bars overflow the chart edges โ the
exact #39141 regression. `codeant` flagged this as Critical and it was answered
"fixed," but the diff for this file only removes the `seriesType` arg; no
fallback was added. Note the new `default: true` does **not** rescue existing
charts โ it only applies when the value is `null` (`getControlState.ts:126`),
and the inherited `initialValue` returns the store
d `false`, so even re-opening/re-saving in Explore keeps it `false`. Also:
porting the Mixed `hasOwnProperty` fallback only covers charts that *omit* the
flag; charts that serialized `xAxisForceCategorical: false` need a form_data
migration to truly restore protection. **regression test:** `transformProps`
for an unstacked bar chart, numeric coltype x-axis, `formData` without (or
`false`) `xAxisForceCategorical` โ assert `echartOptions.xAxis.type ===
AxisType.Category`. (This mirrors the test already added for Mixed at
`MixedTimeseries/transformProps.test.ts`; it will fail today.)
### ๐ก Should-fix
- **CI red** โ `lint-frontend` and `pre-commit (current)` both fail (exit
2). Likely cause: `test/MixedTimeseries/controlPanel.test.ts:19` is an 85-char
single-line import (`import { ControlPanelState, isCustomControlItem } from
'@superset-ui/chart-controls';`) exceeding prettier's 80-char `printWidth`. Run
`pre-commit run --all-files` and push the reformat; must be green before merge.
- **Test/coverage asymmetry** โ Mixed got a regression test for the
omitted-flag fallback, but the regular Bar `transformProps` path (where the fix
is missing) has none. Adding the test above surfaces the gap and guards it
going forward.
### ๐ Praise
- `src/MixedTimeseries/controlPanel.tsx:73-113` โ the `initialValue` logic
correctly preserves an explicit user value while defaulting bar-series mixed
charts to categorical, and the `hasOwnProperty` runtime fallback (with its
comment) cleanly protects legacy Mixed payloads. Apply the same rigor to the
regular Bar path.
<!-- enxdev-review-agent:564bb31 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 564bb31._
--
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]
