codeant-ai-for-open-source[bot] commented on code in PR #39452:
URL: https://github.com/apache/superset/pull/39452#discussion_r3525766869
##########
superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx:
##########
@@ -196,61 +185,80 @@ 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 });
+function WordCloud({
+ data,
+ encoding = {},
+ width,
+ height,
+ rotation = 'flat',
+ sliceId,
+ colorScheme,
+ theme,
+}: FullWordCloudProps) {
+ const [words, setWords] = useState<Word[]>([]);
+ const [scaleFactor] = 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 setWordsIfMounted = useCallback((newWords: Word[]) => {
+ if (isMountedRef.current) {
+ setWords(newWords);
}
- }
-
- update() {
- const { data, encoding } = this.props;
+ }, []);
+
+ 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) {
+ setWordsIfMounted(cloudWords);
+ } else {
+ generateCloud(
+ encoder,
+ currentScaleFactor + SCALE_FACTOR_STEP,
+ isValid,
+ );
+ }
+ })
+ .start();
Review Comment:
**Suggestion:** The asynchronous layout callback can commit stale results
after props change because there is no run-id/cancellation guard per update
cycle. If a previous `cloudLayout` finishes after a newer render started,
`setWords` can overwrite the latest words with outdated data. Track an
incrementing request token (or stop previous layout instances) and ignore `end`
callbacks that do not match the latest token. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Word Cloud may display stale words after rapid changes.
- ⚠️ Users see inconsistent word positions under fast interactions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The Word Cloud visualization type is registered in
`src/visualizations/presets/MainPreset.ts:27` via `new
WordCloudChartPlugin().configure({
key: VizType.WordCloud })`, and is also used in the `ChartDataProvider`
story at
`packages/superset-ui-core/src/chart/stories/ChartDataProvider.stories.tsx:40-48`.
2. When a user opens a Word Cloud chart (either in Explore or through the
`ChartDataProvider` story’s `SuperChart` at
`ChartDataProvider.stories.tsx:51-59`),
Superset loads `WordCloudChartPlugin` from
`plugins/plugin-chart-word-cloud/src/plugin/index.ts:49-57`, whose
`loadChart` callback
`() => import('../chart/WordCloud')` mounts the functional `WordCloud`
component.
3. The `WordCloud` component in
`plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx:188-199`
initializes `words`
state and, through the `update` callback at lines 260-278 and the
`useEffect` at lines
29-45, calls `generateCloud` (lines 229-258) whenever its props (`data`,
`encoding`,
`width`, `height`, `rotation`) change due to new query results from
`transformProps` at
`plugins/plugin-chart-word-cloud/src/plugin/transformProps.ts:37-84`.
4. If the user makes multiple rapid changes (for example quickly adjusting
rotation or
metrics so `transformProps` emits different `data` and `encoding` before the
previous
layout finishes), each change starts a new `cloudLayout()` run in
`generateCloud` at lines
235-255. Because the `.on('end', (cloudWords: Word[]) => { ... })` callback
at lines
244-254 does not check a per-run token or cancel previous layouts, an
earlier layout for
stale props can finish after a later one and still call
`setWordsIfMounted(cloudWords)` at
line 246, overwriting the current `words` state with outdated positions and
causing the
rendered Word Cloud to briefly show content from an earlier configuration.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9219fb3f2f674e549f6e962cfa098b84&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=9219fb3f2f674e549f6e962cfa098b84&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:** 244:255
**Comment:**
*Race Condition: The asynchronous layout callback can commit stale
results after props change because there is no run-id/cancellation guard per
update cycle. If a previous `cloudLayout` finishes after a newer render
started, `setWords` can overwrite the latest words with outdated data. Track an
incrementing request token (or stop previous layout instances) and ignore `end`
callbacks that do not match the latest token.
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=f35401188bb0d4dd4da1461a2fbbb4d7e2de005dcb56d827c896af8023698534&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=f35401188bb0d4dd4da1461a2fbbb4d7e2de005dcb56d827c896af8023698534&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx:
##########
@@ -262,73 +270,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;
+
Review Comment:
**Suggestion:** The update gate omits theme-dependent inputs, so changing
theme values used by the encoder defaults (such as font family) will not
trigger recomputation of word positions. This leaves layout stale under theme
changes. Include theme-derived dependencies in the update condition (or remove
the manual gate and rely on effect dependencies). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Word Cloud layout ignores font changes after theme switch.
- ⚠️ Visual inconsistency between Word Cloud and overall theme.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The `WordCloud` component in
`plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx:188-197` is
exported as
`withTheme(WordCloud)` at lines 81-81, so it receives a `theme:
SupersetTheme` prop from
Superset’s global theming system whenever the application theme changes
(e.g., switching
between light and dark themes).
2. The encoder factory `createEncoder` at `WordCloud.tsx:211-221` constructs
a
`SimpleEncoder` using theme-derived defaults, specifically
`theme.colorTextLabel` and
`theme.fontFamily`, which influence visual properties and are passed into
d3-cloud via
`.text`, `.font`, `.fontWeight`, and `.fontSize` callbacks in
`generateCloud` at lines
239-243, thus affecting both styling and layout.
3. Word positions are recomputed via the `update` callback at
`WordCloud.tsx:260-278`,
which ultimately calls `generateCloud`. However, the `useEffect` at
`WordCloud.tsx:29-45`
only calls `update()` when `shouldUpdate` (lines 32-38) detects changes in
`prevProps.data`, `prevProps.encoding`, `prevProps.width`,
`prevProps.height`, or
`prevProps.rotation`; the `theme` prop is not included in `prevPropsRef` and
is therefore
ignored by the gate.
4. When the global Superset theme changes and `withTheme` injects a new
`theme` object
into `WordCloud`, `createEncoder` and `update` acquire new identities
causing the effect
to re-run, but `shouldUpdate` still evaluates to false because `prevProps`
match the
current `data`, `encoding`, `width`, `height`, and `rotation`. As a result
`update()` is
not invoked, no new layout is generated for the updated theme defaults, and
the existing
`words` (including their stored `font` and positioning) remain tied to the
old theme,
leaving the Word Cloud visually out of sync with the new theme configuration.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c86ba812beaf492488106a9becff238c&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=c86ba812beaf492488106a9becff238c&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:** 292:298
**Comment:**
*Logic Error: The update gate omits theme-dependent inputs, so changing
theme values used by the encoder defaults (such as font family) will not
trigger recomputation of word positions. This leaves layout stale under theme
changes. Include theme-derived dependencies in the update condition (or remove
the manual gate and rely on effect dependencies).
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=b30eb0727980e01a74d68b5101685f558d64c00edca6736827c3dee772e79466&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=b30eb0727980e01a74d68b5101685f558d64c00edca6736827c3dee772e79466&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx:
##########
@@ -196,61 +185,80 @@ 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 });
+function WordCloud({
+ data,
+ encoding = {},
+ width,
+ height,
+ rotation = 'flat',
+ sliceId,
+ colorScheme,
+ theme,
+}: FullWordCloudProps) {
+ const [words, setWords] = useState<Word[]>([]);
+ const [scaleFactor] = useState(1);
Review Comment:
**Suggestion:** The scale factor is read-only and never updated, so retries
that increase `currentScaleFactor` only affect layout computation but not the
SVG viewBox. This can clip words positioned outside the base 1x viewport when
the algorithm expands to fit top terms. Keep a setter and persist the accepted
scale factor when a layout is chosen. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Enlarged word clouds can be partially clipped in viewport.
- ⚠️ Some top words may be invisible in large datasets.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The Word Cloud visualization is wired into Superset via `new
WordCloudChartPlugin().configure({ key: VizType.WordCloud })` in
`src/visualizations/presets/MainPreset.ts:27`, and the plugin’s `loadChart`
in
`plugins/plugin-chart-word-cloud/src/plugin/index.ts:49-57` loads
`chart/WordCloud.tsx`.
2. In `plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx:198-200`, the
functional
component initializes `const [scaleFactor] = useState(1);` and later uses
this value to
compute `viewBoxWidth = width * scaleFactor` and `viewBoxHeight = height *
scaleFactor` at
lines 52-54 of the tail section (311-313 in the file), which define the SVG
viewBox used
in the rendered chart.
3. The `update` callback at `WordCloud.tsx:260-278` builds an encoder and
invokes
`generateCloud(encoder, 1, ...)` at line 273. Inside `generateCloud` (lines
229-258), the
d3-cloud layout is configured with `.size([width * currentScaleFactor,
height *
currentScaleFactor])` at line 236 and, if the resulting `cloudWords` do not
satisfy
`isValid` (ensuring top results are present), the callback recursively calls
`generateCloud` at lines 247-252 with `currentScaleFactor +
SCALE_FACTOR_STEP`,
potentially converging at a `currentScaleFactor > 1`.
4. When such an enlarged layout is accepted (i.e., `isValid(cloudWords)`
succeeds for
`currentScaleFactor > 1` and `setWordsIfMounted(cloudWords)` at line 246
updates `words`),
there is still no `setScaleFactor` call anywhere in `WordCloud.tsx`
(confirmed by
searching for `setScaleFactor` and only finding the read-only `scaleFactor`
usages). The
SVG viewBox continues to use `scaleFactor === 1`, so the final word
positions—computed for
a larger canvas—are rendered into a smaller viewport, which can clip words
near the
boundaries for datasets that required layout expansion.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ee610b2a6c814526977e4535518f90b4&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=ee610b2a6c814526977e4535518f90b4&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:** 199:199
**Comment:**
*Incomplete Implementation: The scale factor is read-only and never
updated, so retries that increase `currentScaleFactor` only affect layout
computation but not the SVG viewBox. This can clip words positioned outside the
base 1x viewport when the algorithm expands to fit top terms. Keep a setter and
persist the accepted scale factor when a layout is chosen.
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=cfdbf06a37d032f52ed5f168ec900884163af8fccf73cffacd16c46f7eb86e95&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39452&comment_hash=cfdbf06a37d032f52ed5f168ec900884163af8fccf73cffacd16c46f7eb86e95&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]
