codeant-ai-for-open-source[bot] commented on code in PR #41000: URL: https://github.com/apache/superset/pull/41000#discussion_r3403548110
########## superset-frontend/src/components/ChatMount/index.tsx: ########## @@ -0,0 +1,135 @@ +/** + * 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 { + type ReactElement, + useMemo, + useRef, + useSyncExternalStore, +} from 'react'; +import { t } from '@apache-superset/core/translation'; +import { logging } from '@apache-superset/core/utils'; +import { css, useTheme } from '@apache-superset/core/theme'; +import { ErrorBoundary } from 'src/components/ErrorBoundary'; +import { addDangerToast } from 'src/components/MessageToasts/actions'; +import { store } from 'src/views/store'; +import { + chat, + getActiveChat, + subscribeToChatState, + getChatStateVersion, +} from 'src/core/chat'; + +const CHAT_EDGE_MARGIN = 24; +const PANEL_MODE_WIDTH = 400; + +/** + * Wraps a chat provider in a React component so that ErrorBoundary can catch + * synchronous throws from the provider function itself. Calling `provider()` + * inline (e.g. `{activeChat.panel()}`) would throw outside React's render + * boundary and crash the host. + */ +const ChatRenderer = ({ provider }: { provider: () => ReactElement }) => + provider(); + +const ChatMount = () => { + const theme = useTheme(); + // Notify once per mount; a crash can re-render and would otherwise re-toast. + const crashNotified = useRef(false); + + // The active chat, the open state, and the display mode all live in the + // chat registry. Read via useSyncExternalStore so the mount re-renders + // whenever a chat extension registers/disposes or toggles its state. + const stateVersion = useSyncExternalStore( + subscribeToChatState, + getChatStateVersion, + ); + + const activeChat = useMemo(() => getActiveChat(), [stateVersion]); + const panelOpen = chat.isOpen(); + const mode = chat.getMode(); + + if (!activeChat) { + return null; + } + + const onProviderError = (error: Error) => { + // Fault isolation: contain the crash, log it, surface a one-time + // notification, and leave the slot empty rather than parking a + // persistent error card. + logging.error('[chat] provider crashed', error); + if (!crashNotified.current) { + crashNotified.current = true; + store.dispatch(addDangerToast(t('The chat failed to load.'))); + } + }; + + if (mode === 'panel') { + // Panel mode hides the trigger and docks the panel to the right edge. + // Interim approximation of the "layout slot between header and footer" + // from the chat API contract — the dock overlays the page until the host + // grows a real layout slot and resizer chrome. + if (!panelOpen) { + return null; + } + return ( + <div + data-test="chat-mount" + css={css` + position: fixed; + top: 0; + right: 0; + bottom: 0; + width: ${PANEL_MODE_WIDTH}px; + background: ${theme.colorBgContainer}; + box-shadow: ${theme.boxShadow}; + /* Above dashboard content and the toast layer, below modal dialogs. */ + z-index: ${theme.zIndexPopupBase + 2}; + `} + > + <ErrorBoundary showMessage={false} onError={onProviderError}> + <ChatRenderer provider={activeChat.panel} /> + </ErrorBoundary> + </div> + ); + } + + return ( + <div + data-test="chat-mount" + css={css` + position: fixed; + right: ${CHAT_EDGE_MARGIN}px; + bottom: ${CHAT_EDGE_MARGIN}px; + display: flex; + flex-direction: column; + align-items: flex-end; + gap: ${theme.sizeUnit * 2}px; + /* Above dashboard content and the toast layer, below modal dialogs. */ + z-index: ${theme.zIndexPopupBase + 2}; + `} + > + <ErrorBoundary showMessage={false} onError={onProviderError}> + {panelOpen && <ChatRenderer provider={activeChat.panel} />} + <ChatRenderer provider={activeChat.trigger} /> + </ErrorBoundary> Review Comment: **Suggestion:** The error boundary is not keyed to the active registration, so once a provider throws, the boundary stays in its errored state and keeps rendering `null` even after a different chat is registered or fallback chat becomes active. Add a reset mechanism (for example a `key` tied to the active chat id) so changing chats remounts the boundary and restores rendering. [stale reference] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ After one provider crash, later chats never render again. - ⚠️ Users permanently lose chat functionality until full page reload. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. `ChatMount` in `src/components/ChatMount/index.tsx:82-90`/112-131 renders the active chat providers inside a host-owned `<ErrorBoundary showMessage={false} onError={onProviderError}>`, without any `key` tied to the active registration, so the same boundary instance wraps all chats across registrations. 2. The `ErrorBoundary` implementation in `src/components/ErrorBoundary/index.tsx:24-40` stores an `error` in its state via `componentDidCatch` and, when `error` is non-null and `showMessage` is false, `render()` returns `null` permanently for its children (lines 42-59); there is no logic to reset this state on prop changes. 3. If a chat provider throws during render—either because the provider function itself throws as in the `isolates a chat whose provider function itself throws` test (`src/components/ChatMount/ChatMount.test.tsx:163-176`) or because its rendered tree throws—`ErrorBoundary` catches the error, calls `onProviderError` in `ChatMount` (lines 71-79) to log and toast, and then starts rendering `null` for all chat content. 4. Later, when a different chat is registered via `chat.registerChat` in `src/core/chat/index.ts:94-121` and becomes active (resolved by `getActiveChat` at lines 91-92, with state changes driven by `notifyState` at 72-75), `ChatMount` re-renders (using `useSyncExternalStore` at lines 58-61) with the new `activeChat`, but the `ErrorBoundary` instance is reused with its `error` state still set, so `render()` continues to return `null` and the new chat's trigger/panel never appear even though the mount `<div data-test="chat-mount">` remains present. 5. Because `ChatMount` is wired into the main app shell in `src/views/App.tsx:8-16` (rendered whenever `FeatureFlag.EnableExtensions` is enabled), this sticky error boundary can leave the chat slot blank for the rest of the session after a single provider crash, with no automatic recovery when new chats register. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d52b8caa544546f0b4b0072ed69adb1b&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=d52b8caa544546f0b4b0072ed69adb1b&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/ChatMount/index.tsx **Line:** 127:130 **Comment:** *Stale Reference: The error boundary is not keyed to the active registration, so once a provider throws, the boundary stays in its errored state and keeps rendering `null` even after a different chat is registered or fallback chat becomes active. Add a reset mechanism (for example a `key` tied to the active chat id) so changing chats remounts the boundary and restores rendering. 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%2F41000&comment_hash=40b8f5d100c830ad6d57b7eb1eaa4eb23e411df7ad87100b8d7f22500261a7f3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41000&comment_hash=40b8f5d100c830ad6d57b7eb1eaa4eb23e411df7ad87100b8d7f22500261a7f3&reaction=dislike'>👎</a> ########## superset-frontend/src/components/ChatMount/index.tsx: ########## @@ -0,0 +1,135 @@ +/** + * 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 { + type ReactElement, + useMemo, + useRef, + useSyncExternalStore, +} from 'react'; +import { t } from '@apache-superset/core/translation'; +import { logging } from '@apache-superset/core/utils'; +import { css, useTheme } from '@apache-superset/core/theme'; +import { ErrorBoundary } from 'src/components/ErrorBoundary'; +import { addDangerToast } from 'src/components/MessageToasts/actions'; +import { store } from 'src/views/store'; +import { + chat, + getActiveChat, + subscribeToChatState, + getChatStateVersion, +} from 'src/core/chat'; + +const CHAT_EDGE_MARGIN = 24; +const PANEL_MODE_WIDTH = 400; + +/** + * Wraps a chat provider in a React component so that ErrorBoundary can catch + * synchronous throws from the provider function itself. Calling `provider()` + * inline (e.g. `{activeChat.panel()}`) would throw outside React's render + * boundary and crash the host. + */ +const ChatRenderer = ({ provider }: { provider: () => ReactElement }) => + provider(); Review Comment: **Suggestion:** Calling the provider as a plain function makes any hooks inside the provider run in `ChatRenderer`'s hook context, so switching to another registered chat with a different hook layout can throw hook-order runtime errors. Render the provider as a React component (`<Provider />`) instead of invoking `provider()` directly so each provider has its own component lifecycle and hook state. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Switching chats with different hooks crashes chat rendering. - ⚠️ Runtime hook-order errors confuse extension developers debugging chats. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Chat providers are registered through `chat.registerChat` in `src/core/chat/index.ts:94-121`, where the `RegisteredChat` interface defines `trigger` and `panel` as `() => ReactElement` callbacks (lines 42-49). 2. The host mount `ChatMount` in `src/components/ChatMount/index.tsx:63-65` pulls the active registration from `getActiveChat()` (`src/core/chat/index.ts:91-92`) and renders it via `<ChatRenderer provider={activeChat.panel} />` or `<ChatRenderer provider={activeChat.trigger} />` (lines 105-107, 127-129). 3. `ChatRenderer` is implemented at `src/components/ChatMount/index.tsx:47-48` as a function component that directly calls the provider: `const ChatRenderer = ({ provider }) => provider();`, so any `useState`/`useEffect` calls inside the provider run in `ChatRenderer`'s hook context. 4. When one extension registers a chat provider that uses a certain set/order of hooks inside its provider function, and a later extension registers another provider with a different hook layout via `chat.registerChat` (`src/core/chat/index.ts:94-121`), a re-render of `ChatRenderer` will execute a different sequence of hooks calls in the same component, violating React's rules-of-hooks and causing runtime hook-order errors during render. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2f9eb76352f94e9ca0e252a5fbbfb508&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=2f9eb76352f94e9ca0e252a5fbbfb508&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/ChatMount/index.tsx **Line:** 47:48 **Comment:** *Api Mismatch: Calling the provider as a plain function makes any hooks inside the provider run in `ChatRenderer`'s hook context, so switching to another registered chat with a different hook layout can throw hook-order runtime errors. Render the provider as a React component (`<Provider />`) instead of invoking `provider()` directly so each provider has its own component lifecycle and hook state. 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%2F41000&comment_hash=a422f117770f5982dc0e24c84706a8f38f0666ef0caa0ce03adc1b853ff76ba7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41000&comment_hash=a422f117770f5982dc0e24c84706a8f38f0666ef0caa0ce03adc1b853ff76ba7&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]
