codeant-ai-for-open-source[bot] commented on code in PR #41000: URL: https://github.com/apache/superset/pull/41000#discussion_r3403543191
########## superset-frontend/src/core/chat/index.ts: ########## @@ -0,0 +1,167 @@ +/** + * 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. + */ + +/** + * @fileoverview Host implementation of the `chat` contribution type. + * + * Chat is a dedicated contribution type, not a view: extensions register via + * the public `chat.registerChat()` and the host owns mounting, open/close + * state, and the display mode. Multiple chat extensions may register, but the + * host applies singleton resolution — the most-recently-registered chat is + * active; disposing it falls back to the previous one. + * + * The public namespace (`chat`) is exposed to extensions on + * `window.superset`; the other exports are host-internal accessors for + * ChatMount and are NOT part of the public `@apache-superset/core` API. + */ + +import { ReactElement } from 'react'; +import type { chat as chatApi } from '@apache-superset/core'; +import { Disposable } from '../models'; +import { createEmitter, createEventEmitter } from '../utils'; + +type Chat = chatApi.Chat; +type ChatMode = chatApi.ChatMode; + +/** A registered chat: its descriptor plus the host-mountable providers. */ +export interface RegisteredChat { + /** The chat descriptor passed to `registerChat`. */ + chat: Chat; + /** Renders the collapsed bubble. Hidden by the host in panel mode. */ + trigger: () => ReactElement; + /** Renders the chat panel, mounted per the current {@link ChatMode}. */ + panel: () => ReactElement; +} + +/** Registration order is the singleton-resolution order: last entry wins. */ +const registrations: RegisteredChat[] = []; + +let panelOpen = false; + +const registerEmitter = createEventEmitter<Chat>(); +const unregisterEmitter = createEventEmitter<Chat>(); +const openEmitter = createEventEmitter<void>(); +const closeEmitter = createEventEmitter<void>(); +const resizePanelEmitter = createEventEmitter<{ width: number }>(); +const modeEmitter = createEmitter<ChatMode>('floating'); + +/** + * Monotonic version of the whole chat state (registrations, open state, and + * mode). Bumped on every change so the host UI can re-derive state via + * React's `useSyncExternalStore`. + */ +let stateVersion = 0; +const stateSubscribers = new Set<() => void>(); + +const notifyState = () => { + stateVersion += 1; + stateSubscribers.forEach(fn => fn()); +}; + +export const subscribeToChatState = (listener: () => void): (() => void) => { + stateSubscribers.add(listener); + return () => { + stateSubscribers.delete(listener); + }; +}; + +export const getChatStateVersion = () => stateVersion; + +/** + * Host-internal: resolves the active chat with its providers. + * The most-recently-registered chat wins; when it is disposed the previous + * registration takes over the slot again. + */ +export const getActiveChat = (): RegisteredChat | undefined => + registrations[registrations.length - 1]; + +const registerChat: typeof chatApi.registerChat = ( + chat: Chat, + trigger: () => ReactElement, + panel: () => ReactElement, +): Disposable => { + // Re-registering an id replaces the previous entry and moves it to the + // most-recent position, mirroring the view registry's same-id semantics. + const existingIndex = registrations.findIndex(r => r.chat.id === chat.id); + if (existingIndex !== -1) { + registrations.splice(existingIndex, 1); + } Review Comment: **Suggestion:** Re-registering an existing chat id removes the previous registration but never emits `onDidUnregisterChat` for the removed descriptor, so lifecycle subscribers miss a required unregister signal and can retain stale state tied to the replaced registration. Emit an unregister event when the old entry is replaced. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Chat lifecycle listeners miss unregister when ids are replaced. - ⚠️ Extensions may leak resources tied to stale chat registrations. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In an extension or integration, subscribe to chat lifecycle events using the public API defined in `@apache-superset/core` (see declaration of `onDidRegisterChat` and `onDidUnregisterChat` in `superset-frontend/packages/superset-core/src/chat/index.ts:21–29`); the host wires these to `registerEmitter` and `unregisterEmitter` in `superset-frontend/src/core/chat/index.ts:57–58`. 2. Register an initial chat implementation with id `'acme.chat'` by calling `chat.registerChat({ id: 'acme.chat', name: 'Acme' }, trigger, panel)`, as in `superset-frontend/src/core/chat/index.test.ts:40–43`. This pushes an entry into `registrations` and fires `registerEmitter.fire(chat)` (`index.ts:106–109`), so your `onDidRegisterChat` listener runs. 3. Later, register an updated implementation with the same id `'acme.chat'`, mirroring the tested "re-registering an id replaces the previous registration" behavior in `superset-frontend/src/core/chat/index.test.ts:74–82`, by calling `chat.registerChat({ id: 'acme.chat', name: 'Acme v2' }, trigger2, panel2)`. 4. In the host implementation of `registerChat` (`superset-frontend/src/core/chat/index.ts:94–108`), the code finds the previous registration index and removes it via `registrations.splice(existingIndex, 1)` (`index.ts:101–103`) but does not call `unregisterEmitter.fire(...)` for that removed descriptor; as a result, `onDidUnregisterChat` subscribers never receive an event for the old `'acme.chat'` registration even though it has been logically unregistered and replaced, leaving any extension state tied to the old registration (such as menus, commands, or internal caches) stale. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e22b75b18e404cc49cdfce8cad1741b8&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=e22b75b18e404cc49cdfce8cad1741b8&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/core/chat/index.ts **Line:** 101:104 **Comment:** *Api Mismatch: Re-registering an existing chat id removes the previous registration but never emits `onDidUnregisterChat` for the removed descriptor, so lifecycle subscribers miss a required unregister signal and can retain stale state tied to the replaced registration. Emit an unregister event when the old entry is replaced. 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=eba5e7b4ab445c5c4eb085f962a9d7c3b10fc85d27136e83d9af8ad30ccf24b4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41000&comment_hash=eba5e7b4ab445c5c4eb085f962a9d7c3b10fc85d27136e83d9af8ad30ccf24b4&reaction=dislike'>👎</a> ########## superset-frontend/src/core/chat/index.ts: ########## @@ -0,0 +1,167 @@ +/** + * 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. + */ + +/** + * @fileoverview Host implementation of the `chat` contribution type. + * + * Chat is a dedicated contribution type, not a view: extensions register via + * the public `chat.registerChat()` and the host owns mounting, open/close + * state, and the display mode. Multiple chat extensions may register, but the + * host applies singleton resolution — the most-recently-registered chat is + * active; disposing it falls back to the previous one. + * + * The public namespace (`chat`) is exposed to extensions on + * `window.superset`; the other exports are host-internal accessors for + * ChatMount and are NOT part of the public `@apache-superset/core` API. + */ + +import { ReactElement } from 'react'; +import type { chat as chatApi } from '@apache-superset/core'; +import { Disposable } from '../models'; +import { createEmitter, createEventEmitter } from '../utils'; + +type Chat = chatApi.Chat; +type ChatMode = chatApi.ChatMode; + +/** A registered chat: its descriptor plus the host-mountable providers. */ +export interface RegisteredChat { + /** The chat descriptor passed to `registerChat`. */ + chat: Chat; + /** Renders the collapsed bubble. Hidden by the host in panel mode. */ + trigger: () => ReactElement; + /** Renders the chat panel, mounted per the current {@link ChatMode}. */ + panel: () => ReactElement; +} + +/** Registration order is the singleton-resolution order: last entry wins. */ +const registrations: RegisteredChat[] = []; + +let panelOpen = false; + +const registerEmitter = createEventEmitter<Chat>(); +const unregisterEmitter = createEventEmitter<Chat>(); +const openEmitter = createEventEmitter<void>(); +const closeEmitter = createEventEmitter<void>(); +const resizePanelEmitter = createEventEmitter<{ width: number }>(); +const modeEmitter = createEmitter<ChatMode>('floating'); + +/** + * Monotonic version of the whole chat state (registrations, open state, and + * mode). Bumped on every change so the host UI can re-derive state via + * React's `useSyncExternalStore`. + */ +let stateVersion = 0; +const stateSubscribers = new Set<() => void>(); + +const notifyState = () => { + stateVersion += 1; + stateSubscribers.forEach(fn => fn()); +}; + +export const subscribeToChatState = (listener: () => void): (() => void) => { + stateSubscribers.add(listener); + return () => { + stateSubscribers.delete(listener); + }; +}; + +export const getChatStateVersion = () => stateVersion; + +/** + * Host-internal: resolves the active chat with its providers. + * The most-recently-registered chat wins; when it is disposed the previous + * registration takes over the slot again. + */ +export const getActiveChat = (): RegisteredChat | undefined => + registrations[registrations.length - 1]; + +const registerChat: typeof chatApi.registerChat = ( + chat: Chat, + trigger: () => ReactElement, + panel: () => ReactElement, +): Disposable => { + // Re-registering an id replaces the previous entry and moves it to the + // most-recent position, mirroring the view registry's same-id semantics. + const existingIndex = registrations.findIndex(r => r.chat.id === chat.id); + if (existingIndex !== -1) { + registrations.splice(existingIndex, 1); + } + + const entry: RegisteredChat = { chat, trigger, panel }; + registrations.push(entry); + registerEmitter.fire(chat); + notifyState(); + + return new Disposable(() => { + const index = registrations.indexOf(entry); + if (index === -1) { + // Already removed — replaced by a same-id registration or disposed twice. + return; + } + registrations.splice(index, 1); + unregisterEmitter.fire(chat); + notifyState(); + }); Review Comment: **Suggestion:** Disposing the active registration only removes it from `registrations` and never reconciles `panelOpen`, so if the last chat is disposed while open, `isOpen()` remains true with no active chat and no close event is fired. When removing the final active chat, also close the panel state and emit `onDidClose`. [stale reference] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ chat.isOpen remains true after last chat disposed. - ⚠️ onDidClose listeners never fire when open chat unregistered. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. An extension registers a chat and stores the Disposable, using `chat.registerChat(descriptor, trigger, panel)` as in `superset-frontend/src/core/chat/index.test.ts:40–43`. This adds an entry to `registrations` and sets it as the active chat (`getActiveChat` in `index.ts:91–92`). 2. The extension or user opens the chat by calling `chat.open()`, which runs the host `open` implementation in `superset-frontend/src/core/chat/index.ts:126–131`, setting `panelOpen = true`, firing `openEmitter`, and causing `chat.isOpen()` to return `true`. `ChatMount` reads `chat.isOpen()` and `getActiveChat()` to render the panel (see `superset-frontend/src/components/ChatMount/index.tsx:63–66` and panel rendering paths at lines 82–89 and 127–129). 3. While the panel is still open, the extension is unloaded or disabled and calls `dispose()` on its registration Disposable; this executes the callback returned from `registerChat` at `superset-frontend/src/core/chat/index.ts:111–120`, which removes the entry from `registrations` and fires `unregisterEmitter.fire(chat)` followed by `notifyState()`. 4. After disposal, `chat.getChat()` and `getActiveChat()` both return `undefined` (similar to the expectation in `index.test.ts:35–38` and `index.test.ts:97–99`), but `panelOpen` remains `true` because the dispose path does not call `close()` or mutate `panelOpen`; consequently, `chat.isOpen()` still returns `true` and `onDidClose` listeners (wired to `closeEmitter` in `index.ts:60,133–137`) never fire, leaving any extension logic that reacts to close events or relies on `isOpen()` inconsistent with the actual UI, which now has no mountable chat. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ac506036499a44758880bbb63f74eb5d&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=ac506036499a44758880bbb63f74eb5d&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/core/chat/index.ts **Line:** 111:120 **Comment:** *Stale Reference: Disposing the active registration only removes it from `registrations` and never reconciles `panelOpen`, so if the last chat is disposed while open, `isOpen()` remains true with no active chat and no close event is fired. When removing the final active chat, also close the panel state and emit `onDidClose`. 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=706a0ab536f420f3ab8dec022771ba54b59de6f63db1286bc0026fb77fb8b9ee&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41000&comment_hash=706a0ab536f420f3ab8dec022771ba54b59de6f63db1286bc0026fb77fb8b9ee&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]
