codeant-ai-for-open-source[bot] commented on code in PR #40440: URL: https://github.com/apache/superset/pull/40440#discussion_r3304452076
########## superset-frontend/src/components/ChatbotMount/index.tsx: ########## @@ -0,0 +1,84 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useState, useEffect } from 'react'; +import { SupersetClient } from '@superset-ui/core'; +import { css, useTheme } from '@apache-superset/core/theme'; +import { ErrorBoundary } from 'src/components/ErrorBoundary'; +import { getActiveChatbot } from 'src/core/chatbot'; +import { subscribeToLocation } from 'src/core/views'; +import { CHATBOT_LOCATION } from 'src/views/contributions'; + +const CHATBOT_EDGE_MARGIN = 24; + +const ChatbotMount = () => { + const theme = useTheme(); + const [adminSelectedId, setAdminSelectedId] = useState<string | null>(null); + const [enabledMap, setEnabledMap] = useState<Record<string, boolean>>({}); + const [activeChatbot, setActiveChatbot] = useState(() => + getActiveChatbot(null, {}), + ); + + useEffect(() => { + let cancelled = false; + SupersetClient.get({ endpoint: '/api/v1/extensions/settings' }) + .then(({ json }) => { + if (cancelled) return; + 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. + }); + return () => { + cancelled = true; + }; + }, []); + + useEffect( + () => + subscribeToLocation(CHATBOT_LOCATION, () => + setActiveChatbot(getActiveChatbot(adminSelectedId, enabledMap)), + ), + [adminSelectedId, enabledMap], + ); + + if (!activeChatbot) { + return null; + } + + return ( + <div + data-test="chatbot-mount" + css={css` + position: fixed; + right: ${CHATBOT_EDGE_MARGIN}px; + bottom: ${CHATBOT_EDGE_MARGIN}px; + /* Above dashboard content and the toast layer, below modal dialogs. */ + z-index: ${theme.zIndexPopupBase + 2}; + `} + > + <ErrorBoundary>{activeChatbot.provider()}</ErrorBoundary> Review Comment: **Suggestion:** The provider is invoked eagerly during `ChatbotMount` render, so if an extension's provider function itself throws, the exception is raised before `ErrorBoundary` can mount and it will bubble up to the host render tree. Render the provider as a child component boundary target (instead of calling it inline) so provider-level failures are actually isolated. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Misbehaving chatbot provider can crash ChatbotMount host render. - ⚠️ Undermines intended isolation tested in ChatbotMount tests. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In `superset-frontend/src/components/ChatbotMount/ChatbotMount.test.tsx`, add a new test similar to `isolates a failing chatbot so it does not crash the host` (lines 77–90) but with a provider that throws synchronously instead of a child component: `const provider = () => { throw new Error('chatbot blew up'); };` registered via `views.registerView` as shown in lines 54–63. 2. In that new test, call `render(<ChatbotMount />);` as done in existing tests at `ChatbotMount.test.tsx:33`, `48`, and `71`, which mounts `ChatbotMount` from `superset-frontend/src/components/ChatbotMount/index.tsx`. 3. During initial render of `ChatbotMount` (file `index.tsx` lines 29–82), `getActiveChatbot` from `superset-frontend/src/core/chatbot/index.ts:59–86` resolves the active chatbot and returns its `provider: () => ReactElement`, which is stored in `activeChatbot` state at lines 33–35. 4. Still in the same render pass, React evaluates the JSX at `index.tsx:79`, executing `activeChatbot.provider()` inline inside `<ErrorBoundary>{activeChatbot.provider()}</ErrorBoundary>`. Because the provider throws before returning a React element, the exception occurs while rendering `ChatbotMount` itself (a parent), not a descendant, so the `ErrorBoundary` from `src/components/ErrorBoundary` never sees it; the `render(<ChatbotMount />)` call in the test will throw, demonstrating that a misbehaving chatbot provider can crash the host despite the intended isolation. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=34793928376c4b2da8cf71a7240eda68&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=34793928376c4b2da8cf71a7240eda68&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:** 79:79 **Comment:** *Possible Bug: The provider is invoked eagerly during `ChatbotMount` render, so if an extension's provider function itself throws, the exception is raised before `ErrorBoundary` can mount and it will bubble up to the host render tree. Render the provider as a child component boundary target (instead of calling it inline) so provider-level failures are actually isolated. 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%2F40440&comment_hash=f789bfb8aa3e49ad73f5d024fe09fc8f5f8b37dd1226e49bc1cee2474defc02a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40440&comment_hash=f789bfb8aa3e49ad73f5d024fe09fc8f5f8b37dd1226e49bc1cee2474defc02a&reaction=dislike'>👎</a> ########## superset-frontend/src/components/ChatbotMount/index.tsx: ########## @@ -0,0 +1,84 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useState, useEffect } from 'react'; +import { SupersetClient } from '@superset-ui/core'; +import { css, useTheme } from '@apache-superset/core/theme'; +import { ErrorBoundary } from 'src/components/ErrorBoundary'; +import { getActiveChatbot } from 'src/core/chatbot'; +import { subscribeToLocation } from 'src/core/views'; +import { CHATBOT_LOCATION } from 'src/views/contributions'; + +const CHATBOT_EDGE_MARGIN = 24; + +const ChatbotMount = () => { + const theme = useTheme(); + const [adminSelectedId, setAdminSelectedId] = useState<string | null>(null); + const [enabledMap, setEnabledMap] = useState<Record<string, boolean>>({}); + const [activeChatbot, setActiveChatbot] = useState(() => + getActiveChatbot(null, {}), + ); Review Comment: **Suggestion:** The mount resolves and renders a chatbot immediately with `null` admin settings, then later replaces it after the async settings fetch; with multiple registered chatbots this can show the wrong (non-admin-selected or disabled) chatbot briefly. Defer initial resolution until settings load (or an explicit "settings unavailable" fallback decision) to avoid rendering an incorrect active chatbot. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Chatbot mount can briefly show admin-disabled chatbot. - ⚠️ Admin-selected chatbot not honored during initial page paint. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Register two chatbot views using the public views API, following the pattern in `superset-frontend/src/components/ChatbotMount/ChatbotMount.test.tsx:54–63` where `views.registerView` from `superset-frontend/src/core/views/index.ts:24–44` is called twice with ids `'first.chatbot'` and `'second.chatbot'` at `CHATBOT_LOCATION` from `src/views/contributions.ts:25–31`. 2. Configure the backend (or a test mock of `SupersetClient.get` imported at `ChatbotMount/index.tsx:20`) so that the `/api/v1/extensions/settings` endpoint returns `json.result.active_chatbot_id === 'second.chatbot'` and `json.result.enabled === { 'first.chatbot': false, 'second.chatbot': true }`, matching the shape read in `index.tsx:42–43`. 3. Render `<ChatbotMount />` as in `ChatbotMount.test.tsx:33/48/71`. Before the async settings fetch resolves, the `useState` initializer at `index.tsx:33–35` calls `getActiveChatbot(null, {})`, which delegates to `getActiveChatbot` in `superset-frontend/src/core/chatbot/index.ts:59–86`. With `adminSelectedId` undefined and an empty `enabledMap`, `getActiveChatbot` does not respect the admin-selected id or disabled flags and simply picks the first registered id (`candidates[0]` at lines 76–79), so the UI initially renders the first chatbot even though it is disabled and not admin-selected. 4. Once `SupersetClient.get` resolves in the effect at `ChatbotMount/index.tsx:37–47`, the component updates `adminSelectedId` and `enabledMap` state and recomputes `setActiveChatbot(getActiveChatbot(id, enabled))`, now correctly resolving `'second.chatbot'`. This causes `ChatbotMount` to switch from the first chatbot's UI to the second, demonstrating a brief period where the wrong (disabled or non-admin-selected) chatbot is rendered before settings load. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a440a8865d3146dfb9ab1a4c2faca1e9&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=a440a8865d3146dfb9ab1a4c2faca1e9&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:** 33:35 **Comment:** *Logic Error: The mount resolves and renders a chatbot immediately with `null` admin settings, then later replaces it after the async settings fetch; with multiple registered chatbots this can show the wrong (non-admin-selected or disabled) chatbot briefly. Defer initial resolution until settings load (or an explicit "settings unavailable" fallback decision) to avoid rendering an incorrect active chatbot. 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%2F40440&comment_hash=ec1830484373cc3f291f83eb3b0f15cc97605b39a8c12c7f53292a478f375082&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40440&comment_hash=ec1830484373cc3f291f83eb3b0f15cc97605b39a8c12c7f53292a478f375082&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]
