codeant-ai-for-open-source[bot] commented on code in PR #40916:
URL: https://github.com/apache/superset/pull/40916#discussion_r3383349904
##########
superset-frontend/src/components/ChatbotMount/index.tsx:
##########
@@ -68,12 +68,13 @@ const ChatbotMount = () => {
useEffect(() => {
// Settings fetch failure is non-fatal: the store keeps its empty default,
- // which getActiveChatbot treats as "all enabled, no admin pin".
+ // which getActiveChatbot treats as "no admin pin" (falls back to the
+ // first-registered chatbot).
loadExtensionSettings().catch(() => {});
}, []);
const activeChatbot = useMemo(
- () => getActiveChatbot(settings.active_chatbot_id, settings.enabled),
+ () => getActiveChatbot(settings.active_chatbot_id),
Review Comment:
**Suggestion:** This computes the active chatbot before the settings fetch
has completed, so on initial mount it can briefly render the first-registered
chatbot even when an admin-pinned chatbot exists. That causes incorrect
provider initialization and possible side effects from the wrong extension
before the async settings response arrives. Keep an explicit "settings loaded"
state (or an undefined sentinel distinct from `null`) and defer chatbot
resolution/rendering until the first settings load completes. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Chatbot bubble initially mounts non-admin-selected provider.
- ⚠️ Wrong chatbot provider may perform unintended initialization actions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable extensions so `ChatbotMount` is rendered from `App.tsx` at
`superset-frontend/src/views/App.tsx:18-25`, where
`{isFeatureEnabled(FeatureFlag.EnableExtensions) && <ChatbotMount />}`
mounts the chatbot
bubble inside `ExtensionsStartup`.
2. Configure two chatbot extensions so they register views in
`CHATBOT_LOCATION` in
registration order `['first.chatbot', 'second.chatbot']`, matching the
pattern used in
`getActiveChatbot` tests at
`superset-frontend/src/core/chatbot/index.test.ts:49-62`.
3. Configure `/api/v1/extensions/settings` to return
`json.result.active_chatbot_id =
"second.chatbot"` so that `loadExtensionSettings()` in
`superset-frontend/src/core/extensions/index.ts:86-89` will eventually set
`settings.active_chatbot_id = "second.chatbot"` (admin-pinned chatbot).
4. Load the UI in a fresh browser session so `ExtensionsStartup` initializes
and renders
children (`superset-frontend/src/extensions/ExtensionsStartup.tsx:44-121`);
on the first
render of `ChatbotMount`
(`superset-frontend/src/components/ChatbotMount/index.tsx:51-64`),
`useSyncExternalStore`
returns the default `settings` with `active_chatbot_id: null`
(`EMPTY_SETTINGS` at
`src/core/extensions/index.ts:46-55`), so `activeChatbot` is computed as
`getActiveChatbot(settings.active_chatbot_id)` (`index.tsx:76-77`) which
calls
`getActiveChatbot(null)` and falls back to the first-registered chatbot
(`registeredIds[0]` in `src/core/chatbot/index.ts:62-75), rendering
`first.chatbot` until
`loadExtensionSettings()` resolves and updates `settings.active_chatbot_id`
to
`"second.chatbot"`, at which point `useMemo` recomputes and switches to the
admin-pinned
chatbot, demonstrating a transient render and initialization of the wrong
provider.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4b68619e6dbc4bb09f96688c59431496&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=4b68619e6dbc4bb09f96688c59431496&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/ChatbotMount/index.tsx
**Line:** 77:77
**Comment:**
*Logic Error: This computes the active chatbot before the settings
fetch has completed, so on initial mount it can briefly render the
first-registered chatbot even when an admin-pinned chatbot exists. That causes
incorrect provider initialization and possible side effects from the wrong
extension before the async settings response arrives. Keep an explicit
"settings loaded" state (or an undefined sentinel distinct from `null`) and
defer chatbot resolution/rendering until the first settings load completes.
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%2F40916&comment_hash=46b70efa00b220e9392e74b7677727409977011b3d29a46521ed5e52997463bc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40916&comment_hash=46b70efa00b220e9392e74b7677727409977011b3d29a46521ed5e52997463bc&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]
