codeant-ai-for-open-source[bot] commented on code in PR #40434:
URL: https://github.com/apache/superset/pull/40434#discussion_r3303519242
##########
superset-frontend/src/components/ChatbotMount/index.tsx:
##########
@@ -39,24 +26,34 @@ import { CHATBOT_LOCATION } from 'src/views/contributions';
const CHATBOT_EDGE_MARGIN = 24;
-/**
- * Renders the active chatbot extension into a fixed bottom-right slot.
- *
- * Mounted once at the app root so the bubble persists across routes.
- * Re-resolves when the chatbot registry changes (extension activated or
- * deactivated at runtime via the P1.A lifecycle contract).
- * Renders null when no chatbot extension is registered.
- */
const ChatbotMount = () => {
const theme = useTheme();
- const [activeChatbot, setActiveChatbot] = useState(getActiveChatbot);
+ const [adminSelectedId, setAdminSelectedId] = useState<string | null>(null);
+ const [enabledMap, setEnabledMap] = useState<Record<string, boolean>>({});
+ const [activeChatbot, setActiveChatbot] = useState(() =>
+ getActiveChatbot(null, {}),
+ );
+
+ useEffect(() => {
+ SupersetClient.get({ endpoint: '/api/v1/extensions/settings' })
+ .then(({ json }) => {
+ const id = json.result?.active_chatbot_id ?? null;
+ const enabled: Record<string, boolean> = json.result?.enabled ?? {};
+ setAdminSelectedId(id);
+ setEnabledMap(enabled);
+ setActiveChatbot(getActiveChatbot(id, enabled));
+ })
+ .catch(() => {
+ // Settings fetch failure is non-fatal โ fall back to
first-to-register.
+ });
+ }, []);
Review Comment:
**Suggestion:** The async settings request has no unmount cleanup, so if the
component unmounts before the request resolves, the `.then()` path can still
call state setters on an unmounted component. Add a mounted/cancelled guard (or
abortable request handling) in the effect cleanup before calling
`setAdminSelectedId`, `setEnabledMap`, and `setActiveChatbot`. [missing cleanup]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Chatbot bubble mount logs React state-update warnings.
- โ ๏ธ ChatbotMount tests may emit noisy React warnings.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. In `superset-frontend/src/views/App.tsx:16-27`, the root `App` component
renders
`<ChatbotMount />` inside `<ExtensionsStartup>` and the React Router, so
`ChatbotMount`
mounts when the SPA loads and unmounts when the React root is torn down.
2. On mount, `ChatbotMount`'s first `useEffect` at
`superset-frontend/src/components/ChatbotMount/index.tsx:37-49` runs once
(empty
dependency array) and calls `SupersetClient.get({ endpoint:
'/api/v1/extensions/settings'
})`, starting an async HTTP request.
3. Before the HTTP request resolves (e.g., due to slow network or server
delay), unmount
the React tree by navigating away or tearing down the root in tests (e.g.,
`render(<ChatbotMount />)` then letting Testing Library unmount in
`ChatbotMount.test.tsx:32-36` and `:38-52` after each test).
4. When the `SupersetClient.get` promise resolves after unmount, its `.then`
handler at
`index.tsx:39-45` still executes `setAdminSelectedId`, `setEnabledMap`, and
`setActiveChatbot`, causing React to perform state updates on an unmounted
component and
emit the standard "Can't perform a React state update on an unmounted
component" warning
and potential test noise; adding an effect cleanup guard (e.g., `let
isMounted = true;
return () => { isMounted = false; };`) would prevent these updates after
unmount.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e8bcbd153a3b483c8261a15dd20fa714&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e8bcbd153a3b483c8261a15dd20fa714&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/src/components/ChatbotMount/index.tsx
**Line:** 37:49
**Comment:**
*Missing Cleanup: The async settings request has no unmount cleanup, so
if the component unmounts before the request resolves, the `.then()` path can
still call state setters on an unmounted component. Add a mounted/cancelled
guard (or abortable request handling) in the effect cleanup before calling
`setAdminSelectedId`, `setEnabledMap`, and `setActiveChatbot`.
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%2F40434&comment_hash=e70a02fdc2d75b9a0140194618f8e8eade64d75963177537738fbdddbdabed9d&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40434&comment_hash=e70a02fdc2d75b9a0140194618f8e8eade64d75963177537738fbdddbdabed9d&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]
