codeant-ai-for-open-source[bot] commented on code in PR #40434: URL: https://github.com/apache/superset/pull/40434#discussion_r3303532411
########## superset-frontend/src/core/navigation/index.ts: ########## @@ -0,0 +1,102 @@ +/** + * 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. + */ + +/** + * Host-internal implementation of the `navigation` namespace. + * + * Backed by browser location — no Redux dependency. + * The app shell calls `notifyPageChange(pathname)` whenever the route changes. + */ + +import type { navigation as navigationApi } from '@apache-superset/core'; +import { Disposable } from '../models'; + +type PageType = navigationApi.PageType; +type PageContext = navigationApi.PageContext; + +const listeners = new Set<(ctx: PageContext) => void>(); + +function derivePageType(pathname: string): PageType { + if (pathname.startsWith('/superset/dashboard/')) return 'dashboard'; + if (pathname.startsWith('/explore/')) return 'explore'; + if (pathname.startsWith('/superset/explore/')) return 'explore'; + if (pathname.startsWith('/chart/add')) return 'explore'; + if (pathname.startsWith('/sqllab/')) return 'sqllab'; + if (pathname.startsWith('/dataset/')) return 'dataset'; + if (pathname.startsWith('/superset/welcome/')) return 'home'; + return 'other'; +} + +function extractEntityId(pathname: string, pageType: PageType): number | null { + if (pageType === 'dashboard') { + const m = pathname.match(/\/superset\/dashboard\/(\d+)/); + return m ? parseInt(m[1], 10) : null; + } + if (pageType === 'dataset') { + const m = pathname.match(/\/dataset\/(\d+)/); + return m ? parseInt(m[1], 10) : null; + } + return null; Review Comment: **Suggestion:** The page context never extracts an entity id for Explore routes, so `entityId` is always `null` on chart edit pages even when a chart id is present. This breaks the documented navigation contract that includes chart ids and will cause extensions to lose chart-level context. [incomplete implementation] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ⚠️ Chatbot extensions lose chart identifier on Explore pages. - ⚠️ Context-aware extensions cannot distinguish individual Explore charts. - ⚠️ Future navigation-based features misinterpret Explore entity context. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the Explore page for an existing chart via React route `/explore/` or `/superset/explore/p` implemented in `superset-frontend/src/views/routes.tsx:178-184`, which renders `ExplorePage` from `superset-frontend/src/pages/Chart/index.tsx:134`. 2. On first render, `ExtensionsStartup` (`superset-frontend/src/extensions/ExtensionsStartup.tsx:63-80`) runs its `useEffect` and calls `notifyPageChange(location.pathname)` with `location.pathname === '/explore/'` or `'/superset/explore/p'`, without any chart id in the path. 3. Inside the navigation host implementation, `derivePageType()` in `superset-frontend/src/core/navigation/index.ts:35-43` returns `'explore'` for these paths, and `extractEntityId()` at `46-55` only handles `'dashboard'` and `'dataset'` page types, returning `null` for `'explore'`. 4. Any extension (or a developer via the browser console) calling `window.superset.navigation.getCurrentPage()` (wired in `ExtensionsStartup` at `superset-frontend/src/extensions/ExtensionsStartup.tsx:91-106` and typed in `packages/superset-core/src/navigation/index.ts:46-54`) on an Explore chart page observes `{ pageType: 'explore', entityId: null }`, violating the documented contract that `entityId` carries the primary entity id (dashboard/chart/dataset) and losing chart-level navigation context. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a6f918afc7844b3b828160f001cd5e86&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=a6f918afc7844b3b828160f001cd5e86&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/navigation/index.ts **Line:** 46:55 **Comment:** *Incomplete Implementation: The page context never extracts an entity id for Explore routes, so `entityId` is always `null` on chart edit pages even when a chart id is present. This breaks the documented navigation contract that includes chart ids and will cause extensions to lose chart-level context. 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%2F40434&comment_hash=dfda7e3501815d18e47b9dd7763aa067627e9149acc3c62750de91d6d92e2a10&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40434&comment_hash=dfda7e3501815d18e47b9dd7763aa067627e9149acc3c62750de91d6d92e2a10&reaction=dislike'>👎</a> ########## superset-frontend/src/core/navigation/index.ts: ########## @@ -0,0 +1,102 @@ +/** + * 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. + */ + +/** + * Host-internal implementation of the `navigation` namespace. + * + * Backed by browser location — no Redux dependency. + * The app shell calls `notifyPageChange(pathname)` whenever the route changes. + */ + +import type { navigation as navigationApi } from '@apache-superset/core'; +import { Disposable } from '../models'; + +type PageType = navigationApi.PageType; +type PageContext = navigationApi.PageContext; + +const listeners = new Set<(ctx: PageContext) => void>(); + +function derivePageType(pathname: string): PageType { + if (pathname.startsWith('/superset/dashboard/')) return 'dashboard'; + if (pathname.startsWith('/explore/')) return 'explore'; + if (pathname.startsWith('/superset/explore/')) return 'explore'; + if (pathname.startsWith('/chart/add')) return 'explore'; + if (pathname.startsWith('/sqllab/')) return 'sqllab'; + if (pathname.startsWith('/dataset/')) return 'dataset'; + if (pathname.startsWith('/superset/welcome/')) return 'home'; + return 'other'; +} + +function extractEntityId(pathname: string, pageType: PageType): number | null { + if (pageType === 'dashboard') { + const m = pathname.match(/\/superset\/dashboard\/(\d+)/); + return m ? parseInt(m[1], 10) : null; + } + if (pageType === 'dataset') { + const m = pathname.match(/\/dataset\/(\d+)/); + return m ? parseInt(m[1], 10) : null; + } + return null; +} + +let currentContext: PageContext = { + pageType: derivePageType(window.location.pathname), + entityId: null, +}; +currentContext.entityId = extractEntityId( + window.location.pathname, + currentContext.pageType, +); + +/** Called by ExtensionsStartup whenever the React Router location changes. */ +export const notifyPageChange = (pathname: string): void => { + const pageType = derivePageType(pathname); + const entityId = extractEntityId(pathname, pageType); + const next: PageContext = { pageType, entityId }; Review Comment: **Suggestion:** Navigation updates are computed from `pathname` only, so context changes that happen via query-string updates on the same route (for example switching Explore chart ids in URL params) are ignored and no page-change event is emitted. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Navigation listeners miss chart switches within Explore. - ⚠️ Chatbot context stays bound to stale chart ids. - ⚠️ Extensions relying on entity changes never update on Explore. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Navigate to the Explore page for a chart using route `/explore/` or `/superset/explore/p`, defined in `superset-frontend/src/views/routes.tsx:178-184`, which renders `ExplorePage` from `src/pages/Chart/index.tsx:134`. 2. `ExplorePage` attaches a history listener (`src/pages/Chart/index.tsx:40-55`) that reloads Explore data whenever the browser history changes (PUSH/POP or certain REPLACE actions), and those history updates typically change only the query string (e.g., `?slice_id=1` → `?slice_id=2`) while keeping `pathname` as `/explore/`. 3. `ExtensionsStartup` (`superset-frontend/src/extensions/ExtensionsStartup.tsx:74-80`) only depends on `location.pathname` and calls `notifyPageChange(location.pathname)` when `location.pathname` changes; pure query-string changes on `/explore/` or `/superset/explore/p` do not re-trigger this effect, so `notifyPageChange` at `src/core/navigation/index.ts:68-80` is never called on chart switches that stay on the same route. 4. As a result, navigation listeners registered via `navigation.onDidChangePage` (API documented in `packages/superset-core/src/navigation/index.ts:78-91`) never receive an event when the user changes charts within Explore by updating URL params, even though the primary entity and chart id have changed, leaving extensions with stale page context. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=13a3e15241ec4bf0b713bc573c046af3&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=13a3e15241ec4bf0b713bc573c046af3&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/navigation/index.ts **Line:** 68:71 **Comment:** *Logic Error: Navigation updates are computed from `pathname` only, so context changes that happen via query-string updates on the same route (for example switching Explore chart ids in URL params) are ignored and no page-change event is emitted. 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%2F40434&comment_hash=556987db3070734f49726874ac47a400778a55eee46060a8777fead0f7682a7a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40434&comment_hash=556987db3070734f49726874ac47a400778a55eee46060a8777fead0f7682a7a&reaction=dislike'>👎</a> ########## 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, + }) + .then(({ json }) => { + setSettings(json.result); + addSuccessToast(t('Settings saved.')); + }) + .catch(() => addDangerToast(t('Failed to save extension settings.'))); + }, + [settings, addDangerToast, addSuccessToast], + ); + + const toggleEnabled = useCallback( + (extensionId: string, enabled: boolean) => { + saveSettings({ enabled: { ...settings.enabled, [extensionId]: enabled } }); + }, + [settings, saveSettings], + ); + + const chatbotExtensions = useMemo(() => { + const chatbotIds = new Set(getRegisteredViewIds(CHATBOT_LOCATION)); + return resourceCollection.filter(ext => chatbotIds.has(ext.id)); + }, [resourceCollection]); Review Comment: **Suggestion:** The chatbot selector is derived from `resourceCollection`, which is only the current paginated list page, not the full extension set. With more than one page of extensions, chatbot candidates outside the current page disappear from the dropdown, making valid defaults impossible to select. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Chatbots on later pages cannot be chosen as default. - ⚠️ Default chatbot setting ignores some registered chatbot views. - ⚠️ Admin UI misrepresents full set of chatbot candidates. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Admin navigates to `/extensions/list/` (route added when `EnableExtensions` is on in `superset-frontend/src/views/routes.tsx:148-152`), which renders `ExtensionsList` (`src/extensions/ExtensionsList.tsx:50-179`) and uses `useListViewResource('extensions', ...)` (`52-62`) to drive the paginated table. 2. `useListViewResource` in `src/views/CRUD/hooks.ts:21-38,93-69` stores only the current page of results in `state.collection`, exposed to callers as `resourceCollection`, while the total number of items lives in `state.count` (exposed as `resourceCount`), and pagination is controlled by `ListView` (`src/components/ListView/ListView.tsx:244-260`) using `data={resourceCollection}` and `count={resourceCount}`. 3. The chatbot dropdown candidates are computed in `ExtensionsList` via `chatbotExtensions = useMemo(() => { const chatbotIds = new Set(getRegisteredViewIds(CHATBOT_LOCATION)); return resourceCollection.filter(ext => chatbotIds.has(ext.id)); }, [resourceCollection]);` at `98-101`, meaning only extensions that are both registered as chatbots and present on the currently visible page of `resourceCollection` are considered. 4. When there are more chatbot-capable extensions than fit on one page (PAGE_SIZE = 25 at `32`), or when a chatbot extension appears on a later page due to sorting/filtering, its id is still returned by `getRegisteredViewIds(CHATBOT_LOCATION)` (`src/core/views/index.ts:120-124`) and used by `ChatbotMount` (`src/components/ChatbotMount/index.tsx:31-45`), but it never appears in `chatbotOptions` (`134-137`) and thus cannot be selected as the "Default chatbot" in the dropdown (`148-163`). ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fa84fcec04db4c448533a61266c7d213&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=fa84fcec04db4c448533a61266c7d213&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:** 98:101 **Comment:** *Logic Error: The chatbot selector is derived from `resourceCollection`, which is only the current paginated list page, not the full extension set. With more than one page of extensions, chatbot candidates outside the current page disappear from the dropdown, making valid defaults impossible to select. 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%2F40434&comment_hash=584fdd11d94d3261503026861ec3360fa9150145bd9afaf47e344f46ad3e1e4e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40434&comment_hash=584fdd11d94d3261503026861ec3360fa9150145bd9afaf47e344f46ad3e1e4e&reaction=dislike'>👎</a> ########## 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, + }) + .then(({ json }) => { + setSettings(json.result); + addSuccessToast(t('Settings saved.')); + }) + .catch(() => addDangerToast(t('Failed to save extension settings.'))); + }, + [settings, addDangerToast, addSuccessToast], Review Comment: **Suggestion:** `saveSettings` builds the payload from the captured `settings` object, so concurrent edits can overwrite each other with stale state (last response wins). Rapid toggles/select changes can drop previously applied updates because each request is composed from an outdated snapshot. [race condition] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Rapid toggles can drop some enabled/disabled changes. - ⚠️ Stored extension flags may not match admin intentions. - ⚠️ Chatbot enablement may appear inconsistent across sessions. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. On `/extensions/list/` (route registered for admins in `superset-frontend/src/views/routes.tsx:148-152`), the `ExtensionsList` component (`src/extensions/ExtensionsList.tsx:50-179`) loads current settings from `/api/v1/extensions/settings` and stores them in local state `settings` (`64-67`). 2. The `toggleEnabled` handler (`91-96`) constructs a new `enabled` map from the captured `settings.enabled` and calls `saveSettings({ enabled: { ...settings.enabled, [extensionId]: enabled } })`, while `saveSettings` (`75-88`) computes `next = { ...settings, ...patch }` and sends the full settings payload via `SupersetClient.put` to `/api/v1/extensions/settings`. 3. If an admin rapidly toggles two different extensions (e.g., A then B) before the first PUT resolves, both `toggleEnabled` calls use the same stale `settings.enabled` snapshot from the previous render, so the first request encodes `{A:false,B:true}` and the second encodes `{A:true,B:false}`, with neither request reflecting both toggles; the backend `update_extension_settings` (`superset/extensions/settings.py:3-21`) treats each PUT as authoritative for those ids, so whichever arrives last wins, potentially leaving either A or B in the wrong state relative to the user's interactions. 4. Because each `.then` handler in `saveSettings` (`81-85`) unconditionally calls `setSettings(json.result)` with the server response, the component state also ends in this last-write-wins configuration, silently dropping one of the toggles despite both interactions having succeeded in the UI. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=74f29165935a46ad863386374bad7016&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=74f29165935a46ad863386374bad7016&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:** 75:88 **Comment:** *Race Condition: `saveSettings` builds the payload from the captured `settings` object, so concurrent edits can overwrite each other with stale state (last response wins). Rapid toggles/select changes can drop previously applied updates because each request is composed from an outdated snapshot. 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%2F40434&comment_hash=90a48e7096c93e5f2bda6954a2c2a04f3f522e55f455ce45bf8ee873a1d5541b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40434&comment_hash=90a48e7096c93e5f2bda6954a2c2a04f3f522e55f455ce45bf8ee873a1d5541b&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]
