codeant-ai-for-open-source[bot] commented on code in PR #40444: URL: https://github.com/apache/superset/pull/40444#discussion_r3304481740
########## superset-frontend/src/core/dashboard/index.ts: ########## @@ -0,0 +1,90 @@ +/** + * 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; Review Comment: **Suggestion:** `getCurrentDashboard()` can return stale dashboard context on non-dashboard pages because it only checks for `dashboardInfo.id`, and dashboard state is kept in the shared store after navigation. This violates the function contract ("undefined when not on Dashboard page") and can make extensions read outdated dashboard identity/filters. Add an explicit page-type/route check before building and returning dashboard context. [stale reference] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Extensions misidentify dashboard when user leaves dashboard page. - ⚠️ Chatbot context uses outdated dashboard filters across navigation. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The global Redux store for the SPA is created in `superset-frontend/src/views/store.ts:132-152,184-185`, where the `dashboardInfo` reducer is registered and `store` is exported for reuse across all routes. 2. When a dashboard is loaded, the `HYDRATE_DASHBOARD` thunk in `superset-frontend/src/dashboard/actions/hydrate.ts:103-112` dispatches an action consumed by `dashboardInfoReducer` in `superset-frontend/src/dashboard/reducers/dashboardInfo.ts:60-66,122-129,194-27`, populating `state.dashboardInfo` (including its `id`) in the shared store. 3. Navigate away to a non-dashboard route (e.g. `/sqllab/` or `/superset/welcome/`); `ExtensionsStartup` in `superset-frontend/src/extensions/ExtensionsStartup.tsx:63-80` calls `notifyPageChange(location.pathname)`, and `navigation` updates `currentPageType` via `derivePageType()` in `superset-frontend/src/core/navigation/index.ts:34-52`, but no reducer clears `state.dashboardInfo`. 4. On that non-dashboard page, an extension or developer calls `window.superset.dashboard.getCurrentDashboard()`, which resolves to `getCurrentDashboard` in `superset-frontend/src/core/dashboard/index.ts:73-74`; this calls `buildDashboardContext()` which only checks `info?.id` at `superset-frontend/src/core/dashboard/index.ts:43-44`, finds the persisted `dashboardInfo.id`, and returns a `DashboardContext` instead of `undefined`, violating the contract documented in `superset-frontend/packages/superset-core/src/dashboard/index.ts:58-61` ("`undefined` when the user is not on a Dashboard page"). ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f84939dfd6c445869e6a4e70a936360c&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=f84939dfd6c445869e6a4e70a936360c&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:** 43:44 **Comment:** *Stale Reference: `getCurrentDashboard()` can return stale dashboard context on non-dashboard pages because it only checks for `dashboardInfo.id`, and dashboard state is kept in the shared store after navigation. This violates the function contract ("undefined when not on Dashboard page") and can make extensions read outdated dashboard identity/filters. Add an explicit page-type/route check before building and returning dashboard 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%2F40444&comment_hash=7b1e6ea678ff692618076639b810c7f63d6e175ed2a016c0d59b614901e4dad3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40444&comment_hash=7b1e6ea678ff692618076639b810c7f63d6e175ed2a016c0d59b614901e4dad3&reaction=dislike'>👎</a> ########## superset-frontend/src/core/explore/index.ts: ########## @@ -0,0 +1,84 @@ +/** + * 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 { SET_DATASOURCE } from 'src/explore/actions/datasourcesActions'; +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; Review Comment: **Suggestion:** `getCurrentChart()` will incorrectly return a chart context even on non-Explore pages because it only checks whether the `explore` slice exists, and that slice is always present in the global store with an initial object. This breaks the API contract (`undefined` when not on Explore) and can expose stale/empty chart context to extensions. Gate this by current route/page type (or an explore-loaded signal) before returning context. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Explore-aware extensions treat empty context as active explore session. - ⚠️ Misaligned API contract complicates extension logic and error handling. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The shared SPA Redux store is configured in `superset-frontend/src/views/store.ts:132-152`, where the `explore` reducer from `superset-frontend/src/explore/reducers/exploreReducer.ts:38-40` is always registered, giving `state.explore` an initial object `{ controls: {}, form_data: {} }` even when Explore has never been visited. 2. On any non-Explore route (e.g. `/superset/welcome/`), the extensions host is initialized by `ExtensionsStartup` in `superset-frontend/src/extensions/ExtensionsStartup.tsx:63-80,101-125`, which attaches `explore` from `src/core` onto `window.superset.explore`. 3. An extension or developer calls `window.superset.explore.getCurrentChart()` on this non-Explore page; this invokes `getCurrentChart` in `superset-frontend/src/core/explore/index.ts:67-68`, which calls `buildChartContext()` at `superset-frontend/src/core/explore/index.ts:40-58`. 4. Inside `buildChartContext()`, `exploreState` is read from `state.explore` at `superset-frontend/src/core/explore/index.ts:41-42`, and since the reducer's initial state is an object, the `if (!exploreState) return undefined;` guard at line 43 never fires; a `ChartContext` with `chartId=null`, `chartName=null`, `vizType=''`, and `datasourceId=null` is returned even off the Explore page, contradicting the documented contract in `superset-frontend/packages/superset-core/src/explore/index.ts:48-51` that `getCurrentChart()` returns `undefined` when the user is not on Explore. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=22a173986f6647378a97385feb1588f4&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=22a173986f6647378a97385feb1588f4&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:** 42:43 **Comment:** *Api Mismatch: `getCurrentChart()` will incorrectly return a chart context even on non-Explore pages because it only checks whether the `explore` slice exists, and that slice is always present in the global store with an initial object. This breaks the API contract (`undefined` when not on Explore) and can expose stale/empty chart context to extensions. Gate this by current route/page type (or an explore-loaded signal) before returning 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%2F40444&comment_hash=c202b95962114dddeac824cc02210ae3e7b196c34cc9f26de608e79439b257bc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40444&comment_hash=c202b95962114dddeac824cc02210ae3e7b196c34cc9f26de608e79439b257bc&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]
