codeant-ai-for-open-source[bot] commented on code in PR #39452:
URL: https://github.com/apache/superset/pull/39452#discussion_r3530988979
##########
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/PairedTTest.tsx:
##########
@@ -114,35 +106,36 @@ const StyledDiv = styled.div`
`}
`;
-class PairedTTest extends PureComponent<PairedTTestProps> {
- static defaultProps = defaultProps;
-
- render() {
- const { className, metrics, groups, data, alpha, pValPrec, liftValPrec } =
- this.props;
-
- return (
- <StyledDiv>
- <div className={`superset-legacy-chart-paired-t-test ${className}`}>
- <div className="paired-ttest-table">
- <div className="scrollbar-content">
- {metrics.map((metric, i) => (
- <TTestTable
- key={i}
- metric={metric}
- groups={groups}
- data={data[metric]}
- alpha={alpha}
- pValPrec={Math.min(pValPrec, 32)}
- liftValPrec={Math.min(liftValPrec, 32)}
- />
- ))}
- </div>
+function PairedTTest({
+ alpha = 0.05,
+ className = '',
+ data,
+ groups,
+ liftValPrec = 4,
+ metrics,
+ pValPrec = 6,
+}: PairedTTestProps) {
+ return (
+ <StyledDiv>
+ <div className={`superset-legacy-chart-paired-t-test ${className}`}>
Review Comment:
**Suggestion:** The wrapper class name uses `paired-t-test` while this
file’s scoped CSS uses `paired_ttest`, so the container-scoped styling selector
does not match and related overflow/layout styles will not apply. Align the
rendered class string with the selector naming convention used in the
stylesheet. [inconsistent naming]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Paired T-Test chart container overflow styling never applies.
⚠️ Paired T-Test tables may not scroll as intended.
⚠️ Visual inconsistency versus other legacy text-based charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The Paired T-Test React component is defined in
`superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/PairedTTest.tsx:109-139`
and exported as the default at line 141, so it is the chart body used by the
`@superset-ui/legacy-plugin-chart-paired-t-test` plugin referenced in
`superset-frontend/src/visualizations/presets/MainPreset.ts:16` via `new
PairedTTestChartPlugin().configure({ key: VizType.PairedTTest })`.
2. Inside `PairedTTest.tsx`, the styled wrapper defines a container-scoped
rule at line
35: `.superset-legacy-chart-paired_ttest .scrollbar-container { overflow:
auto; }`,
meaning overflow styling applies only when a `.scrollbar-container`
descendant is inside
an element with class `superset-legacy-chart-paired_ttest`.
3. The JSX returned from `PairedTTest` at line 120 renders the outer
container with class
`superset-legacy-chart-paired-t-test` (hyphen) instead of
`superset-legacy-chart-paired_ttest` (underscore): `<div
className={`superset-legacy-chart-paired-t-test ${className}`}>`, so no DOM
element ever
receives the exact class `superset-legacy-chart-paired_ttest` that the CSS
selector
targets.
4. Because of this mismatch, when a Paired T-Test chart is rendered (e.g.,
via
`VizType.PairedTTest` in Explore or dashboards as wired in
`MainPreset.ts:16` and
referenced as text-based viz in
`features/reports/ReportModal/index.tsx:10-14` and
`features/alerts/AlertReportModal.tsx:7-11`), the `.scrollbar-container`
inside the table
never matches `.superset-legacy-chart-paired_ttest .scrollbar-container`, so
the intended
overflow/layout styles do not apply, unlike the Horizon chart where the
container class
and scoped selector are consistent (`.superset-legacy-chart-horizon` in both
CSS and JSX
in `plugins/legacy-plugin-chart-horizon/src/HorizonChart.tsx:47-55` and
`:99-100`).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=94a500a35c6b4f62af924dcf73372bf1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=94a500a35c6b4f62af924dcf73372bf1&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/legacy-plugin-chart-paired-t-test/src/PairedTTest.tsx
**Line:** 120:120
**Comment:**
*Inconsistent Naming: The wrapper class name uses `paired-t-test` while
this file’s scoped CSS uses `paired_ttest`, so the container-scoped styling
selector does not match and related overflow/layout styles will not apply.
Align the rendered class string with the selector naming convention used in the
stylesheet.
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%2F39452&comment_hash=9dc9fa06dcc7a65f48f48407c0e94e51ef2d7a142499b070b1a66052995fa2cf&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=9dc9fa06dcc7a65f48f48407c0e94e51ef2d7a142499b070b1a66052995fa2cf&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx:
##########
@@ -262,73 +276,71 @@ class WordCloud extends PureComponent<FullWordCloudProps,
WordCloudState> {
);
const topResults = sortedData.slice(0, topResultsCount);
- this.generateCloud(encoder, 1, (words: Word[]) =>
+ generateCloud(encoder, 1, (cloudWords: Word[]) =>
topResults.every((d: PlainObject) =>
- words.find(({ text }) => encoder.getText(d) === text),
+ cloudWords.find(({ text }) => encoder.getText(d) === text),
),
);
- }
+ }, [data, encoding, createEncoder, generateCloud]);
- generateCloud(
- encoder: SimpleEncoder,
- scaleFactor: number,
- isValid: (word: Word[]) => boolean,
- ) {
- const { data, width, height, rotation } = this.props;
-
- cloudLayout()
- .size([width * scaleFactor, height * scaleFactor])
- .words(data.map((d: Word) => ({ ...d })))
- .padding(5)
- .rotate(ROTATION[rotation] || ROTATION.flat)
- .text((d: PlainObject) => encoder.getText(d))
- .font((d: PlainObject) => encoder.getFontFamily(d))
- .fontWeight((d: PlainObject) => encoder.getFontWeight(d))
- .fontSize((d: PlainObject) => encoder.getFontSize(d))
- .on('end', (words: Word[]) => {
- if (isValid(words) || scaleFactor > MAX_SCALE_FACTOR) {
- this.setWords(words);
- } else {
- this.generateCloud(encoder, scaleFactor + SCALE_FACTOR_STEP,
isValid);
- }
- })
- .start();
- }
+ // Component mount/unmount tracking
+ useEffect(() => {
+ isMountedRef.current = true;
+ return () => {
+ isMountedRef.current = false;
+ };
+ }, []);
- render() {
- const { scaleFactor, words } = this.state;
- const { width, height, encoding, sliceId, colorScheme } = this.props;
-
- const encoder = this.createEncoder(encoding);
-
- const colorFn = CategoricalColorNamespace.getScale(colorScheme);
- const viewBoxWidth = width * scaleFactor;
- const viewBoxHeight = height * scaleFactor;
-
- return (
- <svg
- width={width}
- height={height}
- viewBox={`-${viewBoxWidth / 2} -${viewBoxHeight / 2} ${viewBoxWidth}
${viewBoxHeight}`}
- >
- <g>
- {words.map(w => (
- <text
- key={w.text}
- fontSize={`${w.size}px`}
- fontWeight={w.weight}
- fontFamily={w.font}
- fill={colorFn(encoder.getColor(w as PlainObject), sliceId)}
- textAnchor="middle"
- transform={`translate(${w.x}, ${w.y}) rotate(${w.rotate})`}
- >
- {w.text}
- </text>
- ))}
- </g>
- </svg>
- );
- }
+ // Initial update on mount and when dependencies change
+ useEffect(() => {
+ const prevProps = prevPropsRef.current;
+ const shouldUpdate =
+ !prevProps ||
+ !isEqual(prevProps.data, data) ||
+ !isEqual(prevProps.encoding, encoding) ||
+ prevProps.width !== width ||
+ prevProps.height !== height ||
+ prevProps.rotation !== rotation;
+
+ if (shouldUpdate) {
+ update();
+ }
+
+ prevPropsRef.current = { data, encoding, width, height, rotation };
+ }, [data, encoding, width, height, rotation, update]);
+
+ const encoder = useMemo(
+ () => createEncoder(encoding),
+ [createEncoder, encoding],
+ );
+
+ const colorFn = CategoricalColorNamespace.getScale(colorScheme);
+ const viewBoxWidth = width * scaleFactor;
+ const viewBoxHeight = height * scaleFactor;
+
+ return (
+ <svg
+ width={width}
+ height={height}
+ viewBox={`-${viewBoxWidth / 2} -${viewBoxHeight / 2} ${viewBoxWidth}
${viewBoxHeight}`}
+ >
+ <g>
+ {words.map(w => (
+ <text
+ key={w.text}
+ fontSize={`${w.size}px`}
+ fontWeight={w.weight}
+ fontFamily={w.font}
+ fill={colorFn(encoder.getColor(w as PlainObject), sliceId)}
+ textAnchor="middle"
+ transform={`translate(${w.x}, ${w.y}) rotate(${w.rotate})`}
+ >
+ {w.text}
+ </text>
+ ))}
+ </g>
+ </svg>
+ );
}
export default withTheme(WordCloud);
Review Comment:
**Suggestion:** The class version was a `PureComponent`, but the function
version is exported without `memo`, which removes the shallow render guard and
causes unnecessary rerenders in parent updates even when props are unchanged.
Wrap the themed component with memoization to preserve previous rendering
behavior and avoid extra expensive chart work. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Word Cloud rerenders on parent updates without prop changes.
⚠️ Extra layout work can slow dashboards with Word Clouds.
⚠️ Diverges from previous PureComponent render optimization behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The functional `WordCloud` component is declared in
`superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx:188-344`
and
exported as `export default withTheme(WordCloud);` at line 346, with no
`React.memo` or
other memoization applied to either `WordCloud` or the HOC-wrapped result.
2. The `withTheme` being used is re-exported from Emotion:
`superset-frontend/packages/superset-core/src/theme/index.tsx:36-41` imports
`withTheme`
from `@emotion/react` and re-exports it, and Emotion’s `withTheme` is a
simple HOC that
injects theme via context but does not perform shallow prop comparison or
memoization on
the wrapped component.
3. As a result, whenever the parent chart container re-renders (for example,
due to
generic Explore state updates or surrounding layout changes) but passes the
same `data`,
`encoding`, `width`, `height`, `rotation`, `sliceId`, and `colorScheme`
props down, the
HOC `withTheme(WordCloud)` will still invoke the `WordCloud` function,
re-running its
hooks (including the `useEffect` at `:295-310` that may schedule expensive
`generateCloud`
work) even though props are unchanged.
4. This differs from the prior class-based `PureComponent` semantics
mentioned in the PR
description and in the suggestion: a `PureComponent` under `withTheme` would
have had a
shallow `shouldComponentUpdate` guard, but the new function component lacks
any
equivalent, so the Word Cloud chart does more render and layout work than
necessary on
common UI updates unless it is additionally wrapped in `React.memo` (either
around
`WordCloud` before `withTheme` or around the HOC result).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f256a575c8ca419493c096f647c82c87&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f256a575c8ca419493c096f647c82c87&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/plugin-chart-word-cloud/src/chart/WordCloud.tsx
**Line:** 346:346
**Comment:**
*Performance: The class version was a `PureComponent`, but the function
version is exported without `memo`, which removes the shallow render guard and
causes unnecessary rerenders in parent updates even when props are unchanged.
Wrap the themed component with memoization to preserve previous rendering
behavior and avoid extra expensive chart work.
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%2F39452&comment_hash=b8f7dbde4053ee019f4256a1c945f984c42d0e24a2c1fa4d88eb4f1a193da843&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=b8f7dbde4053ee019f4256a1c945f984c42d0e24a2c1fa4d88eb4f1a193da843&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx:
##########
@@ -196,61 +185,86 @@ class SimpleEncoder {
}
}
-class WordCloud extends PureComponent<FullWordCloudProps, WordCloudState> {
- static defaultProps = defaultProps;
-
- isComponentMounted = false;
-
- createEncoder = (encoding?: Partial<WordCloudEncoding>): SimpleEncoder =>
- new SimpleEncoder(encoding ?? {}, {
- color: this.props.theme.colorTextLabel,
- fontFamily: this.props.theme.fontFamily,
- fontSize: 20,
- fontWeight: 'bold',
- text: '',
- });
-
- constructor(props: FullWordCloudProps) {
- super(props);
- this.state = {
- words: [],
- scaleFactor: 1,
- };
- this.setWords = this.setWords.bind(this);
- }
-
- componentDidMount() {
- this.isComponentMounted = true;
- this.update();
- }
-
- componentDidUpdate(prevProps: WordCloudProps) {
- const { data, encoding, width, height, rotation } = this.props;
- if (
- !isEqual(prevProps.data, data) ||
- !isEqual(prevProps.encoding, encoding) ||
- prevProps.width !== width ||
- prevProps.height !== height ||
- prevProps.rotation !== rotation
- ) {
- this.update();
- }
- }
-
- componentWillUnmount() {
- this.isComponentMounted = false;
- }
-
- setWords(words: Word[]) {
- if (this.isComponentMounted) {
- this.setState({ words });
- }
- }
-
- update() {
- const { data, encoding } = this.props;
+function WordCloud({
+ data,
+ encoding = {},
+ width,
+ height,
+ rotation = 'flat',
+ sliceId,
+ colorScheme,
+ theme,
+}: FullWordCloudProps) {
+ const [words, setWords] = useState<Word[]>([]);
+ const [scaleFactor, setScaleFactor] = useState(1);
+ const isMountedRef = useRef(true);
+
+ // Store previous props for comparison
+ const prevPropsRef = useRef<{
+ data: PlainObject[];
+ encoding: Partial<WordCloudEncoding>;
+ width: number;
+ height: number;
+ rotation: RotationType;
+ } | null>(null);
+
+ const createEncoder = useCallback(
+ (enc?: Partial<WordCloudEncoding>): SimpleEncoder =>
+ new SimpleEncoder(enc ?? {}, {
+ color: theme.colorTextLabel,
+ fontFamily: theme.fontFamily,
+ fontSize: 20,
+ fontWeight: 'bold',
+ text: '',
+ }),
+ [theme.colorTextLabel, theme.fontFamily],
+ );
+
+ const commitLayoutIfMounted = useCallback(
+ (newWords: Word[], newScaleFactor: number) => {
+ if (isMountedRef.current) {
+ setWords(newWords);
+ // Persist the accepted scale factor so the SVG viewBox matches the
+ // canvas the layout was computed for (otherwise enlarged layouts clip)
+ setScaleFactor(newScaleFactor);
+ }
+ },
+ [],
+ );
+
+ const generateCloud = useCallback(
+ (
+ encoder: SimpleEncoder,
+ currentScaleFactor: number,
+ isValid: (word: Word[]) => boolean,
+ ) => {
+ cloudLayout()
+ .size([width * currentScaleFactor, height * currentScaleFactor])
+ .words(data.map((d: Word) => ({ ...d })))
+ .padding(5)
+ .rotate(ROTATION[rotation] || ROTATION.flat)
+ .text((d: PlainObject) => encoder.getText(d))
+ .font((d: PlainObject) => encoder.getFontFamily(d))
+ .fontWeight((d: PlainObject) => encoder.getFontWeight(d))
+ .fontSize((d: PlainObject) => encoder.getFontSize(d))
+ .on('end', (cloudWords: Word[]) => {
+ if (isValid(cloudWords) || currentScaleFactor > MAX_SCALE_FACTOR) {
+ commitLayoutIfMounted(cloudWords, currentScaleFactor);
+ } else {
+ generateCloud(
+ encoder,
+ currentScaleFactor + SCALE_FACTOR_STEP,
+ isValid,
+ );
+ }
+ })
+ .start();
Review Comment:
**Suggestion:** The asynchronous layout pipeline has no run-cancellation or
version guard, so when props change quickly, an older `cloudLayout()`
completion can finish after a newer one and overwrite the latest words with
stale output. Track a monotonically increasing request id (or keep/stop the
previous layout instance) and only commit results from the most recent run.
[race condition]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
❌ Word Cloud can show stale layout after rapid prop changes.
⚠️ Users may see outdated word sizing or positions.
⚠️ Non-deterministic chart behavior complicates debugging and QA.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The Word Cloud chart component is implemented in
`superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx:188-344`
and
exported as `export default withTheme(WordCloud);` at line 346; it is
registered in the
visualization preset as `VizType.WordCloud` via `new
WordCloudChartPlugin().configure({
key: VizType.WordCloud })` in
`superset-frontend/src/visualizations/presets/MainPreset.ts:27`, making this
component the
runtime renderer for Word Cloud charts.
2. When `WordCloud` receives props, the effect at `WordCloud.tsx:295-310`
compares
`prevPropsRef` with the current `{ data, encoding, width, height, rotation
}` and, when
any differ, calls the `update` callback at `:266-284`, which builds an
encoder, derives
`topResults`, and then calls `generateCloud(encoder, 1, isValid)` at line
279.
3. `generateCloud` at `WordCloud.tsx:235-263` constructs a `cloudLayout()`
instance,
configures it with `.size`, `.words(data.map(...))`, `.rotate`, `.text`,
`.font`,
`.fontWeight`, `.fontSize`, and registers an asynchronous `.on('end', ...)`
callback that
either (a) calls `commitLayoutIfMounted(cloudWords, currentScaleFactor)` or
(b)
recursively calls `generateCloud` again with an increased
`currentScaleFactor` if
`isValid` fails, leading to one or more asynchronous layout runs per
`update` invocation.
4. If props change again (e.g., new query results or dimension changes)
before the first
`cloudLayout()` run completes, the effect at `:295-310` triggers another
`update()` and a
second `generateCloud()` call; because neither `generateCloud` nor
`commitLayoutIfMounted`
at `:223-230` track a request id or compare against `prevPropsRef`, the
earlier run’s
`on('end')` can complete after the newer run and still call
`setWords(newWords)` and
`setScaleFactor(newScaleFactor)`, causing the chart state and rendered SVG
(lines 321-343)
to reflect stale word positions for the previous props instead of the latest
`data`/`encoding`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dabaf032c07441c595fae751dea97a05&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=dabaf032c07441c595fae751dea97a05&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/plugin-chart-word-cloud/src/chart/WordCloud.tsx
**Line:** 241:261
**Comment:**
*Race Condition: The asynchronous layout pipeline has no
run-cancellation or version guard, so when props change quickly, an older
`cloudLayout()` completion can finish after a newer one and overwrite the
latest words with stale output. Track a monotonically increasing request id (or
keep/stop the previous layout instance) and only commit results from the most
recent run.
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%2F39452&comment_hash=bb501b2260f342af385a0c055645ab2c2958a08b7334672fd263677e4ecef5c9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=bb501b2260f342af385a0c055645ab2c2958a08b7334672fd263677e4ecef5c9&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]
