EnxDev commented on PR #39452:
URL: https://github.com/apache/superset/pull/39452#issuecomment-4873980629
## EnxDev's Review Agent โ apache/superset#39452 ยท HEAD 801a58e
**comment** โ the Horizon and WordCloud conversions are faithful, but
TTestTable ships real (if positive) behavior changes undocumented in a
`chore(lint)` PR, three of five converted files have zero test coverage, and
the PR body no longer matches the diff.
### ๐ด Functional
-
**`superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:139`**
ยท _Medium_ โ the new effect recomputes lift/p-values whenever `data` changes
and clamps the control index; the class computed only on mount and on row click
(no `componentDidUpdate`), so after a data refresh users previously saw stale
values until they clicked a row. Likely an intentional improvement โ but it's a
silent behavior change in a chore PR; confirm intent and note it in the PR
body. **regression test:** rerender with a new (shorter) `data` prop; assert
values recompute and `control` clamps to `data.length - 1`.
-
**`superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:163`**
ยท _High_ โ the `Number(liftVal)` coercion changes rendered cell classes: on
master `Number.isFinite('33.3333')` is `false` (no coercion), so every
non-control lift cell got class `invalid`; now they get `true`/`false`
(green/red). Same for `getPValueStatus`: `invalid` โ `''`. This restores the
intended color coding, but it's a user-visible styling change hiding in a lint
PR โ worth calling out explicitly. **regression test:** assert the lift `Td`
className is `true`/`false` for finite lift values.
### ๐ก Should-fix
-
**`superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:65`**
โ the new `sumControl === 0` guard renders `NaN` where master rendered
`Infinity` (`Infinity.toFixed(4)` โ `"Infinity"`). Same `invalid` class, but a
visible text change. Also fix `test/TTestTable.test.tsx:189`: the comment
claims `Infinity.toFixed(4)` returns `"NaN"` in jsdom โ it doesn't; the `'NaN'`
comes from this new guard.
- **PR body is stale** โ it claims MapBox/ScatterPlotGlowOverlay
conversions, but no map-box file is in the diff at this HEAD; it also says
sub-PRs target `chore/lint-cleanup-function-components`, while the base is
`master`. Update the description to match.
- **No tests for HorizonChart, HorizonRow, WordCloud** (codecov: 0% patch on
all three, 37% overall). The conversions read as faithful, but hook-conversion
regressions (canvas redraw deps, layout effects) are exactly what a render +
redraw-on-prop-change test would catch.
-
**`superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/PairedTTest.tsx`**
/ **`plugin-chart-word-cloud/src/chart/WordCloud.tsx`** โ both were
`PureComponent` but the new FCs aren't wrapped in `memo()`, while the Horizon
components are. Dropped shallow-prop render bailout โ perf-only here, but
inconsistent; wrap both for parity.
- **CI** โ `playwright-tests-required` failed on today's re-run of the same
HEAD that passed in April; the branch is ~1500 commits behind master, so this
smells like base drift. Rebase and re-run before merge.
### ๐ต Nits
-
`superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:285`
โ `sortConfig`'s `useMemo` sits below the `throw` at line 210, making it a
conditionally-called hook. Not observable (the render errors out anyway), but
hoist it above the guard with the other hooks.
### ๐ Praise
- `test/TTestTable.test.tsx` โ solid new coverage: real assertions on
computed lift/p-values, click-to-change-control interaction, zero-control and
empty-groups edge cases.
- `superset-frontend/plugins/legacy-plugin-chart-horizon/src/HorizonRow.tsx`
โ clean mount/update/unmount โ effect mapping with a complete dependency array.
<!-- enxdev-review-agent:801a58e -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 801a58e._
--
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]
