EnxDev commented on PR #39452: URL: https://github.com/apache/superset/pull/39452#issuecomment-4901020268
## EnxDev's Review Agent — apache/superset#39452 · HEAD 3b39008 **comment** — Careful, well-tested class→FC refactor; the documented intentional fixes are all present and covered. One reply-claimed fix (lift comparator non-finite handling) isn't actually in the diff. _Supersedes my [earlier review](https://github.com/apache/superset/pull/39452#issuecomment-4873980629) on `801a58e`. Resolved since then: `memo()` now wraps PairedTTest/WordCloud, `sortConfig`'s `useMemo` sits above the `throw`, the PR body documents the intentional changes, and all four components now have tests._ Reviewed as a **refactor** — weighted on behavioral equivalence. I audited each conversion against the class original (defaults, lifecycle→effect mapping, deps, `memo`/`PureComponent` parity, injected props) and checked the "Intentional behavior changes" against the diff and their tests. All hold up: - `TTestTable` lift/p-value coercion (`Number.isFinite('33.3333')` was always `false` → every cell `invalid`) — fixed, tested (`colors finite lift cells…`, `Infinity → invalid`). - Data-change recompute + control-index clamp via `controlRef` (avoids a duplicate recompute on row click) — correct, tested (`recomputes and clamps…`). - `WordCloud` persists the accepted `scaleFactor` into the viewBox — fixed, tested (`retries at a larger scale factor…`). - `TTestTable` correctly left unwrapped (was plain `Component`, not `PureComponent`); `withTheme` retained on `WordCloud`; default params match the old `defaultProps`; effect dep arrays are complete; the `WordCloud` update gate correctly excludes `theme` to mirror the old `componentDidUpdate`. ### 🟡 Should-fix - **`legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:1154`** (lift `sortFunction`) — Your reply on the codeant thread says "Added explicit non-finite checks so it returns 0/-1/1 deterministically," but the diff only adds `if (liftA === liftB) return 0`. For `NaN`/`Infinity` lift (zero-sum control → `computeLift` returns `"NaN"`/`"Infinity"`), `liftA === liftB` is `false` and `liftA > liftB` is `false`, so `(NaN, x)` and `(x, NaN)` both return `1` — the antisymmetry violation codeant flagged is still present. Pre-existing (master has no non-finite handling either), so not a regression — but the claimed fix isn't in the code and it's untested. Add `Number.isFinite` guards or drop the claim. **regression test:** sort a Lift% column containing an `Infinity`/`NaN` row, assert stable ordering. ### 🔵 Nits - **`legacy-plugin-chart-horizon/src/HorizonRow.tsx:421`**, **`legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:895`** · _Low_ — `componentDidMount`/`DidUpdate` (sync, pre-paint) became `useEffect` (async, post-paint), so the canvas draw / initial t-test compute now runs after first paint — a one-frame blank canvas / empty table is possible on mount and redraw. `useLayoutEffect` restores pre-paint parity if the flash is observable. - **`legacy-plugin-chart-paired-t-test/src/PairedTTest.tsx:119`** — Renaming the styled selector `paired_ttest` → `paired-t-test` is an undocumented behavior change (not in the intentional-changes list). Likely inert anyway: the DOM renders `scrollbar-content`, not the `.scrollbar-container` the `overflow: auto` rule targets, so the rule stays dead either way. Worth a line in the description if kept. ### 🙌 Praise - `TTestTable.tsx` — The `controlRef` mirror to read the latest control inside the data-change effect without re-running on every click is the right way to avoid duplicate recomputes. Tests assert real outputs (computed lift/p-values, viewBox dims, memo bailout) rather than smoke-testing. <!-- enxdev-review-agent:3b39008 --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 3b39008._ -- 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]
