codeant-ai-for-open-source[bot] commented on code in PR #40433:
URL: https://github.com/apache/superset/pull/40433#discussion_r3303531236
##########
superset-frontend/src/extensions/ExtensionsList.tsx:
##########
@@ -50,6 +61,45 @@ const ExtensionsList: FunctionComponent<ExtensionsListProps>
= ({
addDangerToast,
);
+ const [settings, setSettings] = useState<ExtensionSettings>({
+ active_chatbot_id: null,
+ enabled: {},
+ });
+
+ useEffect(() => {
+ SupersetClient.get({ endpoint: '/api/v1/extensions/settings' })
+ .then(({ json }) => setSettings(json.result))
+ .catch(() => addDangerToast(t('Failed to load extension settings.')));
+ }, [addDangerToast]);
+
+ const saveSettings = useCallback(
+ (patch: Partial<ExtensionSettings>) => {
+ const next = { ...settings, ...patch };
+ SupersetClient.put({
+ endpoint: '/api/v1/extensions/settings',
+ jsonPayload: next,
Review Comment:
**Suggestion:** Saving always sends a full object built from a captured
`settings` snapshot, so overlapping saves from different controls can overwrite
each other (for example, a later toggle request can revert a recently selected
default chatbot). Avoid closure-stale full-payload writes by basing updates on
latest state and/or sending only the changed patch to the backend. [race
condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Rapid edits can silently revert saved chatbot selection.
- ⚠️ Enabled flags may not match latest admin intention.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. On the `/extensions/list/` admin page, `ExtensionsList` loads the current
settings into
local state via a GET to `/api/v1/extensions/settings`
(superset-frontend/src/extensions/ExtensionsList.tsx:64-73) and defines
`saveSettings`
using the then-current `settings` snapshot (lines 75-89).
2. The admin changes the default chatbot using the "Default chatbot"
`<Select>`
(superset-frontend/src/extensions/ExtensionsList.tsx:145-163); its
`onChange` calls
`saveSettings({ active_chatbot_id: (value as string) ?? null })` (lines
153-159). Inside
`saveSettings`, `const next = { ...settings, ...patch };` (line 77) merges
this patch into
the captured `settings` and sends a PUT to `/api/v1/extensions/settings`
with the full
`next` object (lines 78-80).
3. Before the first PUT resolves and updates local state via
`setSettings(json.result)`
(superset-frontend/src/extensions/ExtensionsList.tsx:83), the admin toggles
an extension's
"Enabled" switch. The `toggleEnabled` handler (lines 91-95) calls
`saveSettings({ enabled:
{ ...settings.enabled, [extensionId]: enabled } })`, but this `saveSettings`
closure is
still using the same stale `settings` snapshot from before the chatbot
change, so `next`
is recomputed from the original settings (with the old `active_chatbot_id`)
plus the
updated `enabled` map.
4. Both PUTs are handled by `ExtensionsRestApi.put_settings`
(superset/superset/extensions/api.py:190-215), which delegates to
`update_extension_settings`
(superset/superset/extensions/settings.py:37-54). Since
`update_extension_settings` applies both `active_chatbot_id` and `enabled`
whenever those
keys are present in the body, the second request—built from the stale
snapshot—reapplies
the old `active_chatbot_id` while updating `enabled`, effectively
overwriting the fresh
chatbot selection from the first request and leaving backend settings that
do not reflect
the admin's most recent combination of changes.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8a6b26ac51394ff88d514e1f399504c4&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=8a6b26ac51394ff88d514e1f399504c4&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/extensions/ExtensionsList.tsx
**Line:** 77:80
**Comment:**
*Race Condition: Saving always sends a full object built from a
captured `settings` snapshot, so overlapping saves from different controls can
overwrite each other (for example, a later toggle request can revert a recently
selected default chatbot). Avoid closure-stale full-payload writes by basing
updates on latest state and/or sending only the changed patch to the backend.
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%2F40433&comment_hash=9d8709b4644a601308fff75747afb474fd549b65da6052a7bfcd07520572cd52&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40433&comment_hash=9d8709b4644a601308fff75747afb474fd549b65da6052a7bfcd07520572cd52&reaction=dislike'>👎</a>
##########
superset-frontend/src/extensions/ExtensionsList.tsx:
##########
@@ -58,15 +108,34 @@ const ExtensionsList:
FunctionComponent<ExtensionsListProps> = ({
size: 'lg',
id: 'name',
Cell: ({
- row: {
- original: { name },
- },
+ row: { original: { name } },
}: any) => name,
},
+ {
+ Header: t('Enabled'),
+ accessor: 'enabled',
+ size: 'sm',
+ id: 'enabled',
+ Cell: ({
+ row: { original: { id, enabled } },
+ }: any) => (
+ <Switch
+ data-test="toggle-enabled"
+ checked={settings.enabled[id] ?? enabled}
+ onClick={(checked: boolean) => toggleEnabled(id, checked)}
+ size="small"
+ />
+ ),
Review Comment:
**Suggestion:** The switch fallback uses `enabled` from the extensions list
payload, but `/api/v1/extensions/` does not provide an `enabled` field (it only
returns extension metadata), so untouched rows resolve to `undefined` and
render with an incorrect checked state. Use an explicit default (for example
`true`) or source enabled state solely from settings with a deterministic
fallback. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Extensions admin switches show incorrect enabled state initially.
- ⚠️ Future enforcement of flags may start from wrong values.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. As an admin with `FeatureFlag.EnableExtensions` enabled, navigate to
`/extensions/list/`, which routes to the lazy `Extensions` component
(superset-frontend/src/views/routes.tsx:170-175) and mounts `ExtensionsList`
(superset-frontend/src/extensions/ExtensionsList.tsx:50-53).
2. `ExtensionsList` uses `useListViewResource<Extension>('extensions', ...)`
(superset-frontend/src/extensions/ExtensionsList.tsx:54-62). The hook's
`fetchData` issues
`GET /api/v1/extensions/?q=...` for the `extensions` resource
(superset-frontend/src/views/CRUD/hooks.ts:152-196).
3. The backend handler `ExtensionsRestApi.get_list`
(superset/superset/extensions/api.py:74-120) builds each entry via
`build_extension_data`
(superset/superset/extensions/utils.py:233-255), which returns objects
containing `id`,
`name`, `version`, `description`, `dependencies`, and optional frontend
fields, but no
`enabled` property.
4. In `ExtensionsList` the "Enabled" column renders a `<Switch>` whose
`checked` value is
`settings.enabled[id] ?? enabled`
(superset-frontend/src/extensions/ExtensionsList.tsx:115-125). For any
extension without a
row in the `extension_enabled` table, the settings payload from
`/api/v1/extensions/settings`
(superset-frontend/src/extensions/ExtensionsList.tsx:64-73
and superset/superset/extensions/settings.py:28-35) does not include that
`extension_id`,
so `settings.enabled[id]` is `undefined`, and the API response does not
define `enabled`
at all. Thus `checked` evaluates to `undefined`, and the toggle renders
unchecked even
though `get_list` is documented to "List all enabled extensions"
(superset/superset/extensions/api.py:79-83), causing the UI to misrepresent
the enabled
status for every extension without an explicit flag.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=42833878ead44f21a490563e8d46a1f6&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=42833878ead44f21a490563e8d46a1f6&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/extensions/ExtensionsList.tsx
**Line:** 115:128
**Comment:**
*Api Mismatch: The switch fallback uses `enabled` from the extensions
list payload, but `/api/v1/extensions/` does not provide an `enabled` field (it
only returns extension metadata), so untouched rows resolve to `undefined` and
render with an incorrect checked state. Use an explicit default (for example
`true`) or source enabled state solely from settings with a deterministic
fallback.
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%2F40433&comment_hash=5477cc6b3b80b41f1460840f57c5e5366703f04cde8c3a7dd89103b3d2e007a3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40433&comment_hash=5477cc6b3b80b41f1460840f57c5e5366703f04cde8c3a7dd89103b3d2e007a3&reaction=dislike'>👎</a>
##########
superset-frontend/src/components/ChatbotMount/index.tsx:
##########
@@ -0,0 +1,76 @@
+/**
+ * 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 { useState, useEffect } from 'react';
+import { SupersetClient } from '@superset-ui/core';
+import { css, useTheme } from '@apache-superset/core/theme';
+import { ErrorBoundary } from 'src/components/ErrorBoundary';
+import { getActiveChatbot } from 'src/core/chatbot';
+import { subscribeToLocation } from 'src/core/views';
+import { CHATBOT_LOCATION } from 'src/views/contributions';
+
+const CHATBOT_EDGE_MARGIN = 24;
+
+const ChatbotMount = () => {
+ const theme = useTheme();
+ const [adminSelectedId, setAdminSelectedId] = useState<string | null>(null);
+ const [activeChatbot, setActiveChatbot] = useState(() =>
+ getActiveChatbot(null),
+ );
+
+ useEffect(() => {
+ SupersetClient.get({ endpoint: '/api/v1/extensions/settings' })
+ .then(({ json }) => {
+ const id = json.result?.active_chatbot_id ?? null;
+ setAdminSelectedId(id);
+ setActiveChatbot(getActiveChatbot(id));
+ })
+ .catch(() => {
+ // Settings fetch failure is non-fatal — fall back to
first-to-register.
+ });
Review Comment:
**Suggestion:** The mount reads admin settings only once on initial render,
so changing the default chatbot in the admin page during the same session does
not update the active chatbot bubble until a full reload. Add a reactive
refresh path (e.g., subscribe to settings changes or re-fetch after settings
updates) so the mounted chatbot stays in sync. [stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Chatbot bubble ignores updated admin default selection.
- ⚠️ Admin must reload page to see new chatbot.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the Superset frontend using `App`
(superset-frontend/src/views/App.tsx:48-93),
which always renders `<ChatbotMount />` inside `<ExtensionsStartup>`
(superset-frontend/src/views/App.tsx:57-89).
2. On initial render, `ChatbotMount`
(superset-frontend/src/components/ChatbotMount/index.tsx:29-55) runs its
first
`useEffect`, which GETs `/api/v1/extensions/settings` and sets
`adminSelectedId` and
`activeChatbot` from `json.result?.active_chatbot_id` (lines 36-41); this
effect has an
empty dependency array so it never runs again.
3. As an admin, navigate to `/extensions/list/`, which is wired to the
`Extensions` route
when `FeatureFlag.EnableExtensions` is on
(superset-frontend/src/views/routes.tsx:170-175); this lazy-loads
`ExtensionsList`
(superset-frontend/src/extensions/ExtensionsList.tsx:50-53), where changing
the "Default
chatbot" select (lines 145-163) calls `saveSettings` and sends a PUT to
`/api/v1/extensions/settings` updating `active_chatbot_id`
(superset/superset/extensions/api.py:190-215,
superset/superset/extensions/settings.py:37-45).
4. After the PUT succeeds, observe the chatbot bubble rendered by
`ChatbotMount` in the
bottom-right: it still uses the original `adminSelectedId` from its initial
GET, because
`adminSelectedId` is never updated again and
`subscribeToLocation(CHATBOT_LOCATION, ...)`
(superset-frontend/src/components/ChatbotMount/index.tsx:48-54) only re-runs
`getActiveChatbot(adminSelectedId)` on view-registration changes, not on
settings changes.
The new default chosen in the admin page is not applied until the SPA is
fully reloaded.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c2721a604fde4c178d4ccef3c1962d32&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=c2721a604fde4c178d4ccef3c1962d32&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:** 36:45
**Comment:**
*Stale Reference: The mount reads admin settings only once on initial
render, so changing the default chatbot in the admin page during the same
session does not update the active chatbot bubble until a full reload. Add a
reactive refresh path (e.g., subscribe to settings changes or re-fetch after
settings updates) so the mounted chatbot stays in sync.
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%2F40433&comment_hash=a7305f15a9ad4250b97b7afbc5ac7b84ddf9baf05437d613c2d9d03dd4a194e5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40433&comment_hash=a7305f15a9ad4250b97b7afbc5ac7b84ddf9baf05437d613c2d9d03dd4a194e5&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]
