codeant-ai-for-open-source[bot] commented on code in PR #41205: URL: https://github.com/apache/superset/pull/41205#discussion_r3437430491
########## superset-frontend/src/core/chat/ChatProvider.ts: ########## @@ -0,0 +1,203 @@ +/** + * 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 { ComponentType } from 'react'; +import type { chat as chatApi } from '@apache-superset/core'; +import { + LocalStorageKeys, + getItem, + setItem, +} from 'src/utils/localStorageHelpers'; +import { Disposable } from '../models'; +import { createValueEventEmitter, createEventEmitter } from '../utils'; + +type Chat = chatApi.Chat; +type DisplayMode = chatApi.DisplayMode; + +/** + * Singleton manager for the chat provider. + * Handles registration, open/close state, and display mode. + */ +class ChatProvider { + private static instance: ChatProvider; + + private chat: Chat | undefined; + + private trigger: ComponentType | undefined; + + private panel: ComponentType | undefined; + + private opened: boolean; + + private stateSubscribers = new Set<() => void>(); + + private registerEmitter = createEventEmitter<Chat>(); + + private unregisterEmitter = createEventEmitter<Chat>(); + + private openEmitter = createEventEmitter<void>(); + + private closeEmitter = createEventEmitter<void>(); + + private resizePanelEmitter = createEventEmitter<{ width: number }>(); + + private modeEmitter: ReturnType<typeof createValueEventEmitter<DisplayMode>>; + + private constructor() { + const persisted = getItem(LocalStorageKeys.ChatState, { + open: false, + mode: 'floating', + }); + const mode = ( + persisted.mode === 'panel' ? 'panel' : 'floating' + ) as DisplayMode; + this.opened = persisted.open === true; + this.modeEmitter = createValueEventEmitter<DisplayMode>(mode); + } + + public static getInstance(): ChatProvider { + if (!ChatProvider.instance) { + ChatProvider.instance = new ChatProvider(); + } + return ChatProvider.instance; + } + + public subscribe = (listener: () => void): (() => void) => { + this.stateSubscribers.add(listener); + return () => this.stateSubscribers.delete(listener); + }; + + private notifyState(): void { + setItem(LocalStorageKeys.ChatState, { + open: this.opened, + mode: this.modeEmitter.getCurrent(), + }); + this.stateSubscribers.forEach(fn => fn()); + } + + private closePanel(): void { + this.opened = false; + this.closeEmitter.fire(); + } + + public registerChat( + chat: Chat, + trigger: ComponentType, + panel: ComponentType, + ): Disposable { + if (this.chat) { + // eslint-disable-next-line no-console + console.warn( + `[Superset] Multiple chat extensions registered. Using "${chat.id}"; discarding "${this.chat.id}".`, + ); + if (this.opened) this.closePanel(); + } + + this.chat = chat; + this.trigger = trigger; + this.panel = panel; + this.registerEmitter.fire(chat); Review Comment: **Suggestion:** Replacing an existing chat registration never emits an unregister event for the displaced chat, so listeners subscribed to unregistration events will miss that transition and can keep stale state/resources. Emit `onDidUnregisterChat` for the previous chat before switching to the new one. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Replacing chat never fires unregistration event for previous chat. - ⚠️ Listeners may leak resources or retain stale chat state. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In an extension (or test), subscribe to unregistration events via `chat.onDidUnregisterChat(listener)` exported from `@apache-superset/core` (`superset-frontend/packages/superset-core/src/chat/index.ts` lines 33-36), which is wired to `ChatProvider.onDidUnregisterChat` in `superset-frontend/src/core/chat/index.ts` lines 7-12. 2. Register an initial chat with `chat.registerChat({ id: 'first.chat', name: 'First' }, Trigger1, Panel1)`; this calls `ChatProvider.registerChat` in `superset-frontend/src/core/chat/ChatProvider.ts` lines 99-117, setting `this.chat` to the first descriptor and firing `registerEmitter`. 3. Without disposing the first registration's `Disposable`, register another chat with `chat.registerChat({ id: 'second.chat', name: 'Second' }, Trigger2, Panel2)`; inside `ChatProvider.registerChat` the `if (this.chat)` block at lines 104-110 logs a warning and (if open) calls `closePanel()`, then lines 112-115 replace `this.chat`, `this.trigger`, and `this.panel` with the new registration and fire only `registerEmitter`. 4. Observe that the `listener` passed to `onDidUnregisterChat` is never invoked when the first chat is displaced, even though `getChat()` (via the same API mapping) now returns the second descriptor; the prior chat is effectively unregistered without any corresponding `onDidUnregisterChat` lifecycle event. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4e4b6a768b3940a3abb5d64c5c436130&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=4e4b6a768b3940a3abb5d64c5c436130&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/ChatProvider.ts **Line:** 104:115 **Comment:** *Api Mismatch: Replacing an existing chat registration never emits an unregister event for the displaced chat, so listeners subscribed to unregistration events will miss that transition and can keep stale state/resources. Emit `onDidUnregisterChat` for the previous chat before switching to the new one. 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%2F41205&comment_hash=9dbafcf4b34113fed907a17fbc780e807afbcec8855adcd3c0cdb38de8b84693&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=9dbafcf4b34113fed907a17fbc780e807afbcec8855adcd3c0cdb38de8b84693&reaction=dislike'>👎</a> ########## superset-frontend/src/core/chat/ChatProvider.ts: ########## @@ -0,0 +1,203 @@ +/** + * 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 { ComponentType } from 'react'; +import type { chat as chatApi } from '@apache-superset/core'; +import { + LocalStorageKeys, + getItem, + setItem, +} from 'src/utils/localStorageHelpers'; +import { Disposable } from '../models'; +import { createValueEventEmitter, createEventEmitter } from '../utils'; + +type Chat = chatApi.Chat; +type DisplayMode = chatApi.DisplayMode; + +/** + * Singleton manager for the chat provider. + * Handles registration, open/close state, and display mode. + */ +class ChatProvider { + private static instance: ChatProvider; + + private chat: Chat | undefined; + + private trigger: ComponentType | undefined; + + private panel: ComponentType | undefined; + + private opened: boolean; + + private stateSubscribers = new Set<() => void>(); + + private registerEmitter = createEventEmitter<Chat>(); + + private unregisterEmitter = createEventEmitter<Chat>(); + + private openEmitter = createEventEmitter<void>(); + + private closeEmitter = createEventEmitter<void>(); + + private resizePanelEmitter = createEventEmitter<{ width: number }>(); + + private modeEmitter: ReturnType<typeof createValueEventEmitter<DisplayMode>>; + + private constructor() { + const persisted = getItem(LocalStorageKeys.ChatState, { + open: false, + mode: 'floating', + }); + const mode = ( + persisted.mode === 'panel' ? 'panel' : 'floating' + ) as DisplayMode; + this.opened = persisted.open === true; + this.modeEmitter = createValueEventEmitter<DisplayMode>(mode); + } + + public static getInstance(): ChatProvider { + if (!ChatProvider.instance) { + ChatProvider.instance = new ChatProvider(); + } + return ChatProvider.instance; + } + + public subscribe = (listener: () => void): (() => void) => { + this.stateSubscribers.add(listener); + return () => this.stateSubscribers.delete(listener); + }; + + private notifyState(): void { + setItem(LocalStorageKeys.ChatState, { + open: this.opened, + mode: this.modeEmitter.getCurrent(), + }); + this.stateSubscribers.forEach(fn => fn()); + } + + private closePanel(): void { + this.opened = false; + this.closeEmitter.fire(); + } + + public registerChat( + chat: Chat, + trigger: ComponentType, + panel: ComponentType, + ): Disposable { + if (this.chat) { + // eslint-disable-next-line no-console + console.warn( + `[Superset] Multiple chat extensions registered. Using "${chat.id}"; discarding "${this.chat.id}".`, + ); + if (this.opened) this.closePanel(); + } + + this.chat = chat; + this.trigger = trigger; + this.panel = panel; + this.registerEmitter.fire(chat); + this.notifyState(); + + return new Disposable(() => { + if (this.chat?.id !== chat.id) return; + this.chat = undefined; + this.trigger = undefined; + this.panel = undefined; + this.unregisterEmitter.fire(chat); + if (this.opened) this.closePanel(); + this.notifyState(); + }); Review Comment: **Suggestion:** The disposal guard only compares `id`, so if a new registration reuses the same chat id, disposing the old registration will incorrectly unregister the newer active chat. Track registration identity (for example by comparing object reference or a generated token) instead of just `id` before clearing provider state. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Disposing old registration can unregister latest active chat unexpectedly. - ⚠️ Chat UI becomes unavailable until new registration occurs. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In an extension, import the chat API from `@apache-superset/core` (`superset-frontend/packages/superset-core/src/chat/index.ts`, registerChat declaration around lines 54-57) and call `chat.registerChat({ id: 'acme.chat', name: 'First' }, Trigger1, Panel1)` to register the first chat; keep the returned `Disposable` as `d1`. 2. Without disposing `d1`, call `chat.registerChat({ id: 'acme.chat', name: 'Second' }, Trigger2, Panel2)` a second time (same `id`, different descriptor/components); this flows into `ChatProvider.registerChat` in `superset-frontend/src/core/chat/ChatProvider.ts` lines 99-117 where `this.chat` is updated to the new descriptor and the old one is effectively displaced. 3. Later, dispose the original registration by calling `d1.dispose()`, which invokes the `Disposable` callback defined at `ChatProvider.ts` lines 118-126: `if (this.chat?.id !== chat.id) return;` followed by clearing `this.chat`, `this.trigger`, `this.panel`, firing `unregisterEmitter`, and closing the panel. 4. Because both registrations used the same `chat.id` string, the guard at line 119 succeeds even though `this.chat` now refers to the newer registration, so the dispose of `d1` incorrectly unregisters the second (current) chat: `chat.getChat()` (mapped via `superset-frontend/src/core/chat/index.ts` lines 7-12) now returns `undefined` and any listeners see an unregistration for the active chat while the second registration's `Disposable` is still held. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c7c44c209de845aa8f028030eb749a93&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=c7c44c209de845aa8f028030eb749a93&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/ChatProvider.ts **Line:** 118:126 **Comment:** *Logic Error: The disposal guard only compares `id`, so if a new registration reuses the same chat id, disposing the old registration will incorrectly unregister the newer active chat. Track registration identity (for example by comparing object reference or a generated token) instead of just `id` before clearing provider 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%2F41205&comment_hash=c23b9183cdf2ab05aa18e237fffca0ab384cf069dd4b0218382c7c4eedf63d0f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=c23b9183cdf2ab05aa18e237fffca0ab384cf069dd4b0218382c7c4eedf63d0f&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]
