codeant-ai-for-open-source[bot] commented on code in PR #40434: URL: https://github.com/apache/superset/pull/40434#discussion_r3303539012
########## superset-frontend/packages/superset-core/src/navigation/index.ts: ########## @@ -0,0 +1,91 @@ +/** + * 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 Navigation namespace for Superset extensions (P3). + * + * Exposes stable routing and page-surface context to extensions. + * Extensions use this namespace to react to page changes without polling. + */ + +import { Event } from '../common'; + +/** + * The set of top-level application surfaces the chatbot is aware of. + * `'other'` covers any route not explicitly enumerated. + */ +export type PageType = + | 'dashboard' + | 'explore' + | 'sqllab' + | 'dataset' + | 'home' + | 'other'; + +/** + * Lightweight page descriptor: the current surface and the focused entity id, + * if the surface has one (dashboard id, chart id, dataset id). Does not embed + * full entity payloads — use the surface-specific namespace for those. + */ +export interface PageContext { + pageType: PageType; + /** + * The numeric id of the primary entity on this page, if applicable. + * `null` when the surface has no focused entity, or when the entity is + * addressed by a non-numeric slug (e.g. dashboard accessed via slug URL). + */ + entityId: number | null; +} Review Comment: **Suggestion:** The public navigation contract documents that `entityId` covers chart ids on Explore pages, but the host implementation only extracts ids for dashboard and dataset routes, so consumers will receive `null` on Explore and break if they rely on this documented behavior. Align the contract and implementation by either adding Explore chart-id extraction or removing chart-id guarantees from this API description. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Extensions misinterpret Explore context due to missing chart id. - ⚠️ Chatbot navigation logic for Explore pages becomes unreliable. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open a saved chart in Explore so the browser URL is `/superset/explore/?slice_id=123` or similar, which is routed through React Router and surfaced via `useLocation()` in `ExtensionsStartup` at `superset-frontend/src/extensions/ExtensionsStartup.tsx:63-80`. 2. On the first render after navigation, `ExtensionsStartup` calls `notifyPageChange(location.pathname)` when `location.pathname` changes at `superset-frontend/src/extensions/ExtensionsStartup.tsx:74-80`, passing a pathname like `/superset/explore/`. 3. `notifyPageChange()` in `superset-frontend/src/core/navigation/index.ts:68-80` calls `derivePageType(pathname)` (lines 35-42), which returns `'explore'` for `/superset/explore/`, then calls `extractEntityId(pathname, pageType)` (lines 46-55), which only extracts ids for `'dashboard'` and `'dataset'` and returns `null` for `'explore'`. 4. An extension using the public navigation contract from `@apache-superset/core` (defined at `superset-frontend/packages/superset-core/src/navigation/index.ts:41-54`) and calling `navigation.getCurrentPage()` receives `{ pageType: 'explore', entityId: null }` even though the API comment documents `entityId` as the focused entity id "dashboard id, chart id, dataset id", so any consumer relying on a chart id for Explore pages will break. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=27d4d5319b264cc5bacb0fd3d513efbc&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=27d4d5319b264cc5bacb0fd3d513efbc&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/packages/superset-core/src/navigation/index.ts **Line:** 41:54 **Comment:** *Api Mismatch: The public navigation contract documents that `entityId` covers chart ids on Explore pages, but the host implementation only extracts ids for dashboard and dataset routes, so consumers will receive `null` on Explore and break if they rely on this documented behavior. Align the contract and implementation by either adding Explore chart-id extraction or removing chart-id guarantees from this API description. 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=11eed0069f6ef6a6f49aa3e35466503d2951f0f4e59397719fb9289024688422&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40434&comment_hash=11eed0069f6ef6a6f49aa3e35466503d2951f0f4e59397719fb9289024688422&reaction=dislike'>👎</a> ########## superset-frontend/src/core/explore/index.ts: ########## @@ -0,0 +1,82 @@ +/** + * 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 `explore` namespace. + * + * Wraps Redux explore state and normalizes it into the stable `ChartContext` + * contract. Extensions must not depend on the Redux slice structure directly. + */ + +import type { explore as exploreApi } from '@apache-superset/core'; +import { HYDRATE_EXPLORE } from 'src/explore/actions/hydrateExplore'; +import { + SET_FORM_DATA, + UPDATE_CHART_TITLE, +} from 'src/explore/actions/exploreActions'; +import { store, RootState } from 'src/views/store'; +import { AnyListenerPredicate } from '@reduxjs/toolkit'; +import { createActionListener } from '../utils'; + +type ChartContext = exploreApi.ChartContext; + +function buildChartContext(): ChartContext | undefined { + const state = store.getState(); + const exploreState = (state as any).explore; + if (!exploreState) return undefined; + + const { slice, datasource, controls } = exploreState; + const vizType: string = + (controls?.viz_type?.value as string) ?? + exploreState.form_data?.viz_type ?? + ''; + + return { + chartId: slice?.slice_id ?? null, + chartName: slice?.slice_name ?? null, Review Comment: **Suggestion:** `chartName` is read from `slice.slice_name`, but title edits are stored in `explore.sliceName` by the reducer, so emitted chart context can carry a stale name after `UPDATE_CHART_TITLE`. Read the title from the same state field that the update action writes to. [incorrect variable usage] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Explore extensions see stale chart titles after rename. - ⚠️ Chatbot may reference outdated chart names in responses. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. On an Explore page for a saved chart, Redux `explore` state is managed by `exploreReducer` in `superset-frontend/src/explore/reducers/exploreReducer.ts:45-78`, which defines `ExploreState` with both `slice?: Slice | null` and a separate `sliceName?: string` field (lines 56-62). 2. The user edits the chart title in the Explore UI, which dispatches `updateChartTitle(sliceName)` from `superset-frontend/src/explore/actions/exploreActions.ts:115-118` with the new title string. 3. `exploreReducer` handles `UPDATE_CHART_TITLE` at `superset-frontend/src/explore/reducers/exploreReducer.ts:554-559`, updating `state.sliceName = typedAction.sliceName` but leaving `state.slice?.slice_name` unchanged, so the canonical current title lives in `sliceName`, not in the `slice` object. 4. The extension-side chart context is built by `buildChartContext()` in `superset-frontend/src/core/explore/index.ts:39-57`, which sets `chartName: slice?.slice_name ?? null` (line 52) and ignores `sliceName`, meaning that even though `onDidChangeChart` is fired for `UPDATE_CHART_TITLE` (the predicate includes it at lines 60-63), listeners receive a `ChartContext` whose `chartName` remains the old title, contrary to the public contract in `superset-frontend/packages/superset-core/src/explore/index.ts:35-40` that `chartName` is the current display name of the saved chart. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=05a8b19864b94b609bd200739432ed94&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=05a8b19864b94b609bd200739432ed94&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/explore/index.ts **Line:** 52:52 **Comment:** *Incorrect Variable Usage: `chartName` is read from `slice.slice_name`, but title edits are stored in `explore.sliceName` by the reducer, so emitted chart context can carry a stale name after `UPDATE_CHART_TITLE`. Read the title from the same state field that the update action writes to. 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=809991b0ccc74b9bc9f8fe3bed75de222d0134ce1835e7cc63fc735ce4aa382a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40434&comment_hash=809991b0ccc74b9bc9f8fe3bed75de222d0134ce1835e7cc63fc735ce4aa382a&reaction=dislike'>👎</a> ########## superset-frontend/src/core/dashboard/index.ts: ########## @@ -0,0 +1,86 @@ +/** + * 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 `dashboard` namespace. + * + * Wraps Redux dashboardInfo and dataMask state and normalizes them into the + * stable `DashboardContext` contract. Extensions must not depend on the Redux + * slice structure directly. + */ + +import type { dashboard as dashboardApi } from '@apache-superset/core'; +import { HYDRATE_DASHBOARD } from 'src/dashboard/actions/hydrate'; +import { + UPDATE_DATA_MASK, + SET_DATA_MASK_FOR_FILTER_CHANGES_COMPLETE, +} from 'src/dataMask/actions'; +import { store, RootState } from 'src/views/store'; +import { AnyListenerPredicate } from '@reduxjs/toolkit'; +import { createActionListener } from '../utils'; + +type DashboardContext = dashboardApi.DashboardContext; +type FilterValue = dashboardApi.FilterValue; + +function buildDashboardContext(): DashboardContext | undefined { + const state = store.getState(); + const info = (state as any).dashboardInfo; + if (!info?.id) return undefined; + + const nativeFilters = (state as any).nativeFilters?.filters ?? {}; + const dataMask = (state as any).dataMask ?? {}; + + const filters: FilterValue[] = Object.entries(dataMask) + .filter(([id]) => id in nativeFilters) + .map(([id, mask]: [string, any]) => ({ + filterId: id, + label: nativeFilters[id]?.name ?? id, + value: mask?.filterState?.value ?? null, + })); Review Comment: **Suggestion:** The dashboard context currently includes native filters even when they have no applied value, and coerces missing values to `null`, which violates the declared contract that only active/applied filters are exposed. Filter out entries with no applied value before mapping them into `filters`. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Extensions misread dashboard filter state as always active. - ⚠️ Chatbot explanations may reference filters user has cleared. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. On a dashboard with native filters configured, the dashboard hydration and filter configuration logic in `superset-frontend/src/dataMask/reducer.ts:90-131` calls `getInitialDataMask()` for each filter, which initializes `filterState: {}` (no `value` key) for every native filter (lines 90-100 and 112-117). 2. When the dashboard is loaded and before the user selects any filter values, Redux `state.dataMask` therefore contains entries for each native filter id whose `filterState` object has no `value` field (i.e. the filter has no applied value yet). 3. The host `buildDashboardContext()` implementation in `superset-frontend/src/core/dashboard/index.ts:41-61` constructs `filters` by iterating `Object.entries(dataMask)` and only filtering by `id in nativeFilters` (lines 49-51), then mapping every such entry to a `FilterValue` where `value` is computed as `mask?.filterState?.value ?? null` (line 54), producing `FilterValue` objects with `value: null` for filters that are not applied. 4. An extension calling `dashboard.getCurrentDashboard()` from the public API defined at `superset-frontend/packages/superset-core/src/dashboard/index.ts:43-56` receives a `DashboardContext.filters` array that includes filters with `value: null` even though the contract explicitly states "Only includes filters that have a value applied" (lines 51-55), so the extension cannot reliably distinguish active filters from filters with no selection. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=731a48f88bb143b98fe9b7cc93f874c8&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=731a48f88bb143b98fe9b7cc93f874c8&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/dashboard/index.ts **Line:** 49:55 **Comment:** *Api Mismatch: The dashboard context currently includes native filters even when they have no applied value, and coerces missing values to `null`, which violates the declared contract that only active/applied filters are exposed. Filter out entries with no applied value before mapping them into `filters`. 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=0e4781d785fa2a288a5499430fbc6f82d1624c4fec0f237787fd3228c1197356&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40434&comment_hash=0e4781d785fa2a288a5499430fbc6f82d1624c4fec0f237787fd3228c1197356&reaction=dislike'>👎</a> ########## superset-frontend/src/core/explore/index.ts: ########## @@ -0,0 +1,82 @@ +/** + * 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 `explore` namespace. + * + * Wraps Redux explore state and normalizes it into the stable `ChartContext` + * contract. Extensions must not depend on the Redux slice structure directly. + */ + +import type { explore as exploreApi } from '@apache-superset/core'; +import { HYDRATE_EXPLORE } from 'src/explore/actions/hydrateExplore'; +import { + SET_FORM_DATA, + UPDATE_CHART_TITLE, +} from 'src/explore/actions/exploreActions'; +import { store, RootState } from 'src/views/store'; +import { AnyListenerPredicate } from '@reduxjs/toolkit'; +import { createActionListener } from '../utils'; + +type ChartContext = exploreApi.ChartContext; + +function buildChartContext(): ChartContext | undefined { + const state = store.getState(); + const exploreState = (state as any).explore; + if (!exploreState) return undefined; + + const { slice, datasource, controls } = exploreState; + const vizType: string = + (controls?.viz_type?.value as string) ?? + exploreState.form_data?.viz_type ?? + ''; + + return { + chartId: slice?.slice_id ?? null, + chartName: slice?.slice_name ?? null, + vizType, + datasourceId: datasource?.id ?? null, + datasourceName: + datasource?.table_name ?? datasource?.datasource_name ?? null, + }; +} + +const exploreChangePredicate: AnyListenerPredicate<RootState> = action => + action.type === HYDRATE_EXPLORE || + action.type === SET_FORM_DATA || + action.type === UPDATE_CHART_TITLE; Review Comment: **Suggestion:** `onDidChangeChart` does not subscribe to datasource-change actions, so changing the Explore datasource will not emit a chart-context update even though the API contract says datasource changes should trigger this event. Include the datasource-change action type in the predicate so listeners receive updates when datasource changes. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Explore extensions miss datasource switches in chart context. - ⚠️ Chatbot may answer using stale datasource information. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. On an Explore page, a chart is open and Redux `explore` state is managed by `exploreReducer` in `superset-frontend/src/explore/reducers/exploreReducer.ts:45-78`, where `ExploreState.datasource` holds the current datasource. 2. An extension subscribes to chart context changes by calling `explore.onDidChangeChart(listener)` from `@apache-superset/core`, which is implemented in `superset-frontend/src/core/explore/index.ts:68-77` using `createActionListener(exploreChangePredicate, ...)` with `exploreChangePredicate` defined at lines 60-63 to watch only `HYDRATE_EXPLORE`, `SET_FORM_DATA`, and `UPDATE_CHART_TITLE`. 3. The user changes the datasource in the Explore UI, which dispatches `changeDatasource(newDatasource)` from `superset-frontend/src/explore/actions/datasourcesActions.ts:48-55`; this in turn dispatches `updateFormDataByDatasource(prevDatasource, newDatasource)` (line 54). 4. The `exploreReducer` handles `UPDATE_FORM_DATA_BY_DATASOURCE` at `superset-frontend/src/explore/reducers/exploreReducer.ts:281-337`, updating both `state.datasource` (line 323-327) and `state.form_data`, but because `exploreChangePredicate` does not include the `UPDATE_FORM_DATA_BY_DATASOURCE` action type, `createActionListener` never invokes the registered `onDidChangeChart` listeners, so extensions do not see datasourceId/datasourceName changes despite the public contract at `superset-frontend/packages/superset-core/src/explore/index.ts:63-65` documenting that datasource changes should fire this event. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ab8f4455ec914d72b629debd5a475306&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=ab8f4455ec914d72b629debd5a475306&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/explore/index.ts **Line:** 60:63 **Comment:** *Api Mismatch: `onDidChangeChart` does not subscribe to datasource-change actions, so changing the Explore datasource will not emit a chart-context update even though the API contract says datasource changes should trigger this event. Include the datasource-change action type in the predicate so listeners receive updates when datasource changes. 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=0d58a7daebbd74be45a858fcddd7d1b2d62b2380f7f9012c02899f6764286fef&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40434&comment_hash=0d58a7daebbd74be45a858fcddd7d1b2d62b2380f7f9012c02899f6764286fef&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]
