codeant-ai-for-open-source[bot] commented on code in PR #40443:
URL: https://github.com/apache/superset/pull/40443#discussion_r3304497432
##########
superset-frontend/src/core/views/index.ts:
##########
@@ -52,9 +73,12 @@ const registerView: typeof viewsApi.registerView = (
ids.add(id);
locationIndex.set(location, ids);
+ notifyListeners(location);
+
return new Disposable(() => {
viewRegistry.delete(id);
locationIndex.get(location)?.delete(id);
+ notifyListeners(location);
});
Review Comment:
**Suggestion:** Listener notifications are emitted only for the incoming
`location`, so when a view id is re-registered under a different location,
subscribers of the previous location are never notified and can keep stale UI
state. Track the previous location for an existing id and notify both old and
new locations on overwrite/removal. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Admin extensions list can show stale chatbot registrations.
- ⚠️ Location-based listeners remain out-of-sync after id moves.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the frontend so the host views registry in
`src/core/views/index.ts` is
initialized and components subscribe to locations: `ChatbotMount` subscribes
to
`CHATBOT_LOCATION` via `subscribeToLocation` at
`src/components/ChatbotMount/index.tsx:56-60`, and the admin extensions UI
subscribes
similarly in `src/extensions/ExtensionsList.tsx:10-16`.
2. Have an extension register a chatbot view at `CHATBOT_LOCATION` using
`views.registerView` (the host implementation at
`src/core/views/index.ts:63-76` and
public API documented in `packages/superset-core/src/views/index.ts:16-23`),
which adds
the view id to `locationIndex.get(CHATBOT_LOCATION)` and calls
`notifyListeners(CHATBOT_LOCATION)` at `src/core/views/index.ts:76`, causing
subscribers
like `ExtensionsList` (`chatbotRegistryVersion` in
`src/extensions/ExtensionsList.tsx:10-16`) to observe the change.
3. Later, the same extension (or a buggy update of it) re-registers the same
view id under
a different location (e.g. `'sqllab.panels'`) without disposing the original
`Disposable`;
`registerView` overwrites the `viewRegistry` entry with the new `location`
and `provider`
but only calls `notifyListeners(location)` for the new location at
`src/core/views/index.ts:76`, leaving the id still present in the old
location's
`locationIndex` set and emitting no notification for the old location.
4. Subscribers for the old location (e.g. `ExtensionsList` using
`subscribeToLocation(CHATBOT_LOCATION, ...)` at
`src/extensions/ExtensionsList.tsx:10-16`
and recomputing `chatbotExtensions` from
`getRegisteredViewIds(CHATBOT_LOCATION)` at
`src/extensions/ExtensionsList.tsx:48-51`) never receive an event about the
id moving, so
they keep stale state: the admin UI can list a chatbot that no longer truly
resides at
`CHATBOT_LOCATION`, and any logic depending on registration-change
notifications for that
location stays out of sync with the actual `viewRegistry`.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9c0afd1bc7ac4ddba5b9407b4869ad3f&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=9c0afd1bc7ac4ddba5b9407b4869ad3f&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/views/index.ts
**Line:** 76:82
**Comment:**
*Logic Error: Listener notifications are emitted only for the incoming
`location`, so when a view id is re-registered under a different location,
subscribers of the previous location are never notified and can keep stale UI
state. Track the previous location for an existing id and notify both old and
new locations on overwrite/removal.
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%2F40443&comment_hash=d6424215b9522063644fd7ba7d315a314f39dd7f3bc8ec21cc0369cd4a112fd3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40443&comment_hash=d6424215b9522063644fd7ba7d315a314f39dd7f3bc8ec21cc0369cd4a112fd3&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/chatbot/index.ts:
##########
@@ -0,0 +1,87 @@
+/**
+ * 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-internal resolver for the exclusive `superset.chatbot`
+ * contribution area.
+ *
+ * `superset.chatbot` is a singleton contribution area: multiple chatbot
+ * extensions may register a view there, but the host renders exactly one.
+ * This module owns the host-side selection policy.
+ *
+ * This is host-internal infrastructure — it is NOT part of the public
+ * `@apache-superset/core` API. Extensions register via the public
+ * `views.registerView()`; only the host resolves which one is active.
+ */
+
+import { ReactElement } from 'react';
+import { CHATBOT_LOCATION } from 'src/views/contributions';
+import { getRegisteredViewIds, getViewProvider } from 'src/core/views';
+
+/**
+ * The resolved active chatbot: a view id paired with its renderable provider.
+ */
+export interface ActiveChatbot {
+ /** The registered view id of the selected chatbot. */
+ id: string;
+ /** The provider that renders the chatbot's React element. */
+ provider: () => ReactElement;
+}
+
+/**
+ * Resolves which single chatbot extension is currently active.
+ *
+ * Selection policy:
+ * - If no chatbot is registered, returns `undefined` — the corner stays
empty.
+ * - Disabled chatbots (per `enabledMap`) are excluded before selection.
+ * - If `adminSelectedId` matches an enabled registered chatbot, that one
wins.
+ * - Otherwise the first enabled chatbot in registration order is used as a
fallback.
+ *
+ * @param adminSelectedId The id stored in the admin "Default chatbot"
setting, if any.
+ * @param enabledMap Per-extension enabled flags from the admin settings API.
+ * @returns The active chatbot's id and provider, or `undefined` if none.
+ */
+export const getActiveChatbot = (
+ adminSelectedId?: string | null,
+ enabledMap?: Record<string, boolean>,
+): ActiveChatbot | undefined => {
+ const registeredIds = getRegisteredViewIds(CHATBOT_LOCATION);
+ if (registeredIds.length === 0) {
+ return undefined;
+ }
+
+ const candidates = enabledMap
+ ? registeredIds.filter(id => enabledMap[id] !== false)
+ : registeredIds;
+
+ if (candidates.length === 0) {
+ return undefined;
+ }
+
+ const selectedId =
+ adminSelectedId && candidates.includes(adminSelectedId)
+ ? adminSelectedId
+ : candidates[0];
+
+ const provider = getViewProvider(CHATBOT_LOCATION, selectedId);
+ if (!provider) {
+ return undefined;
+ }
Review Comment:
**Suggestion:** The resolver returns `undefined` immediately when the
selected id has no provider, instead of trying the remaining candidates. With
stale ids (for example after id collisions/re-registrations), this makes the
chatbot disappear even when another valid chatbot is registered. Iterate
through candidates and pick the first id that still resolves to a provider.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Chatbot bubble can disappear despite enabled extensions.
- ⚠️ Admin-selected chatbot id silently yields empty UI.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the frontend with extensions enabled so `ExtensionsStartup`
initializes the
extension host and renders `ChatbotMount` (see
`superset-frontend/src/extensions/ExtensionsStartup.tsx:18-25` and
`superset-frontend/src/components/ChatbotMount/index.tsx:29-35`).
2. Have two chatbot extensions register views at `CHATBOT_LOCATION` using
the public API
documented in `packages/superset-core/src/views/index.ts:16-23`, e.g.
similar to the tests
in `src/core/chatbot/index.test.ts:49-62` where `views.registerView` is
called with ids
`first.chatbot` and `second.chatbot` at `'superset.chatbot'`.
3. Introduce a stale id by re-registering the `first.chatbot` id at a
different location
(e.g. `'sqllab.panels'`) without disposing the original registration;
`registerView` at
`src/core/views/index.ts:63-76` overwrites the `viewRegistry` entry to point
to the new
location but leaves `first.chatbot` in the `locationIndex` set for
`CHATBOT_LOCATION`, so
`getRegisteredViewIds(CHATBOT_LOCATION)` (`src/core/views/index.ts:121-123`)
returns
`['first.chatbot', 'second.chatbot']` even though `first.chatbot` is no
longer actually at
that location.
4. Trigger chatbot resolution (for example via the `subscribeToLocation`
callback in
`ChatbotMount` at `src/components/ChatbotMount/index.tsx:56-60`, which calls
`setActiveChatbot(getActiveChatbot(adminSelectedId, enabledMap))`):
`getActiveChatbot`
(`src/core/chatbot/index.ts:59-86`) selects `selectedId='first.chatbot'`,
then
`getViewProvider(CHATBOT_LOCATION, selectedId)`
(`src/core/views/index.ts:109-117`)
returns `undefined` because the `viewRegistry` entry's `location` no longer
matches
`CHATBOT_LOCATION`, so the function returns `undefined` at
`src/core/chatbot/index.ts:81-84` without ever trying `second.chatbot`,
causing the
chatbot bubble to disappear even though a valid second chatbot is still
registered.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2dc5f84a953c40638373fa17a1c1fffe&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=2dc5f84a953c40638373fa17a1c1fffe&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/chatbot/index.ts
**Line:** 81:84
**Comment:**
*Logic Error: The resolver returns `undefined` immediately when the
selected id has no provider, instead of trying the remaining candidates. With
stale ids (for example after id collisions/re-registrations), this makes the
chatbot disappear even when another valid chatbot is registered. Iterate
through candidates and pick the first id that still resolves to a provider.
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%2F40443&comment_hash=adc608cb072e1651d9255095420e00ce3bbb85ece33c1b0ae2e0e90b15b1be83&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40443&comment_hash=adc608cb072e1651d9255095420e00ce3bbb85ece33c1b0ae2e0e90b15b1be83&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]
