codeant-ai-for-open-source[bot] commented on code in PR #40968:
URL: https://github.com/apache/superset/pull/40968#discussion_r3394450588
##########
superset-frontend/src/core/chatbot/index.ts:
##########
@@ -48,31 +48,22 @@ export interface ActiveChatbot {
*
* Selection policy:
* - If no chatbot is registered, returns `undefined` — the corner stays
empty.
- * - If `adminSelectedId` matches a registered chatbot, that one wins.
- * - Otherwise the first-registered chatbot is used as a fallback.
- * The active chatbot pin is set only via the backend DB; when no pin is set
- * (active_chatbot_id is null), the fallback is the first-registered
chatbot.
+ * - Otherwise the most-recently-registered (last-loaded) chatbot wins. When a
+ * second chatbot extension loads, it takes over the singleton bubble.
*
- * @param adminSelectedId The id stored in the DB "Default chatbot" setting,
if any.
* @returns The active chatbot's id and provider, or `undefined` if none.
*/
-export const getActiveChatbot = (
- adminSelectedId?: string | null,
-): ActiveChatbot | undefined => {
+export const getActiveChatbot = (): ActiveChatbot | undefined => {
const registeredIds = getRegisteredViewIds(CHATBOT_LOCATION);
if (registeredIds.length === 0) {
return undefined;
}
- // When the DB pin names a registered candidate, use it; otherwise fall back
- // to the first registered chatbot in registration order.
- // `getRegisteredViewIds` and `getViewProvider` read the same synchronous
- // registry maps, so a candidate id always has a live provider; the final
+ // `getRegisteredViewIds` returns ids in registration order, so the last
entry
+ // is the most-recently-loaded chatbot. `getViewProvider` reads the same
+ // synchronous registry maps, so the id always has a live provider; the final
// guard is cheap defensiveness, not a fallback path.
- const selectedId =
- adminSelectedId && registeredIds.includes(adminSelectedId)
- ? adminSelectedId
- : registeredIds[0];
+ const selectedId = registeredIds[registeredIds.length - 1];
Review Comment:
**Suggestion:** The new last-loaded policy assumes `getRegisteredViewIds()`
is ordered by latest registration event, but that index is backed by a `Set` in
`core/views` where re-registering an existing id does not move it to the end.
If a chatbot with an existing id registers again after another chatbot, this
code can still select the older id order and return the wrong active chatbot.
Update selection to track true recency (for example, refresh ordering on
re-register in the registry, or store a registration sequence/timestamp per
entry and select by that). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Chatbot bubble may show older chatbot after extension reload.
- ⚠️ Extension developers cannot rely on documented last-loaded selection.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Observe the view registry implementation in
`superset-frontend/src/core/views/index.ts`: `locationIndex` is defined as
`const
locationIndex: Map<string, Set<string>> = new Map();` at lines 35–40, and
`registerView`
at lines 63–80 calls `const ids = locationIndex.get(location) ?? new Set();
ids.add(id);
locationIndex.set(location, ids);` without ever removing and re-adding an
existing id at
the same location.
2. Note that `getRegisteredViewIds` in the same file at lines 68–71 returns
`Array.from(ids)` for a given location, so the ordering of ids is the
original insertion
order of the `Set`, and calling `ids.add(existingId)` again does not move
that id to the
end of the sequence.
3. Confirm how the active chatbot is resolved in
`superset-frontend/src/core/chatbot/index.ts`: `getActiveChatbot` at lines
56–69 calls
`getRegisteredViewIds(CHATBOT_LOCATION)` (line 57) and then selects the last
entry via
`const selectedId = registeredIds[registeredIds.length - 1];` (line 66),
with the comment
at lines 49–52 stating the selection policy is "the most-recently-registered
(last-loaded)
chatbot wins."
4. To reproduce the mismatch, add a Jest test alongside the existing ones in
`superset-frontend/src/components/ChatbotMount/ChatbotMount.test.tsx` (which
already
registers multiple chatbots via `views.registerView` at lines 53–67) that:
(a) registers
`{ id: 'first.chatbot' }` at `CHATBOT_LOCATION`, (b) registers `{ id:
'second.chatbot' }`
at `CHATBOT_LOCATION`, and then (c) registers `{ id: 'first.chatbot' }`
again at
`CHATBOT_LOCATION` using `views.registerView` exported from `src/core`. When
this test
renders `<ChatbotMount />` (which calls `getActiveChatbot()` at
`src/components/ChatbotMount/index.tsx:31,45–59`), `getActiveChatbot` still
sees
`getRegisteredViewIds('core.chatbot') === ['first.chatbot',
'second.chatbot']` (because
the `Set` did not reorder), so it returns `selectedId === 'second.chatbot'`
even though
the last registration event was for `'first.chatbot'`, violating the
documented
"last-loaded wins" policy.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=72c04b35a0a149eabf05eef7e429887f&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=72c04b35a0a149eabf05eef7e429887f&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:** 62:66
**Comment:**
*Logic Error: The new last-loaded policy assumes
`getRegisteredViewIds()` is ordered by latest registration event, but that
index is backed by a `Set` in `core/views` where re-registering an existing id
does not move it to the end. If a chatbot with an existing id registers again
after another chatbot, this code can still select the older id order and return
the wrong active chatbot. Update selection to track true recency (for example,
refresh ordering on re-register in the registry, or store a registration
sequence/timestamp per entry and select by that).
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%2F40968&comment_hash=8e9ce3f6cec7beee283feb345f617f5e93d064393d2f86d92192aa9ae98a605f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40968&comment_hash=8e9ce3f6cec7beee283feb345f617f5e93d064393d2f86d92192aa9ae98a605f&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]
