sadpandajoe commented on code in PR #44276: URL: https://github.com/apache/superset/pull/44276#discussion_r4011806919
########## superset-frontend/src/core/dashboard/index.ts: ########## @@ -0,0 +1,105 @@ +/** + * 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 { dashboard as dashboardApi } from '@apache-superset/core'; +import { isNativeFilter } from '@superset-ui/core'; +import type { Divider, Filter } from '@superset-ui/core'; +import { updateComponents } from 'src/dashboard/actions/dashboardLayout'; +import { dashboardInfoChanged } from 'src/dashboard/actions/dashboardInfo'; +import { updateDataMask } from 'src/dataMask/actions'; +import { store, RootState } from 'src/views/store'; + +const getState = () => store.getState() as RootState; + +const requireDashboardId = (): number => { + const { id } = getState().dashboardInfo; + if (id == null) { + throw new Error('No dashboard is currently active'); + } + return id; +}; + +const getDashboardId: typeof dashboardApi.getDashboardId = () => + getState().dashboardInfo.id ?? undefined; + +const getLayout: typeof dashboardApi.getLayout = () => ({ + ...(getState().dashboardLayout.present as Record<string, unknown>), +}); + +const updateLayoutNode: typeof dashboardApi.updateLayoutNode = async ( + nodeId: string, + meta: Record<string, unknown>, +) => { + requireDashboardId(); + const node = getState().dashboardLayout.present[nodeId]; + if (!node) { + throw new Error(`Layout node "${nodeId}" not found`); + } + // UPDATE_COMPONENTS replaces each keyed entry wholesale (it's not a deep + // merge), so the node's other fields must be carried through alongside + // the merged meta. + store.dispatch( + updateComponents({ + [nodeId]: { ...node, meta: { ...node.meta, ...meta } }, + }) as any, + ); +}; + +const getCss: typeof dashboardApi.getCss = () => getState().dashboardInfo.css ?? ''; + +const setCss: typeof dashboardApi.setCss = async (css: string) => { + requireDashboardId(); + store.dispatch(dashboardInfoChanged({ css })); Review Comment: Calling `setCss('')` resolves and makes `getCss()` return an empty string, but `DashboardPage` uses `dashboardInfo.css || dashboard?.css`, so a dashboard with saved CSS immediately reapplies the old stylesheet instead of clearing it. Should the render path distinguish an explicit empty override from an unset value? ########## superset-frontend/src/core/dashboard/index.ts: ########## @@ -0,0 +1,105 @@ +/** + * 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 { dashboard as dashboardApi } from '@apache-superset/core'; +import { isNativeFilter } from '@superset-ui/core'; +import type { Divider, Filter } from '@superset-ui/core'; +import { updateComponents } from 'src/dashboard/actions/dashboardLayout'; +import { dashboardInfoChanged } from 'src/dashboard/actions/dashboardInfo'; +import { updateDataMask } from 'src/dataMask/actions'; +import { store, RootState } from 'src/views/store'; + +const getState = () => store.getState() as RootState; + +const requireDashboardId = (): number => { + const { id } = getState().dashboardInfo; Review Comment: These guards only check the retained Redux slice, so after the in-SPA Dashboard → Explore transition they still return the previous dashboard ID and allow layout/filter/CSS mutations instead of reporting no active dashboard; the reverse leaves Explore APIs pointed at the previous chart. Can these namespaces gate on `navigation.getPage()` or clear their slices on route exit to match the surface-scoped contract? ########## superset-frontend/src/core/explore/index.ts: ########## @@ -0,0 +1,95 @@ +/** + * 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 { explore as exploreApi } from '@apache-superset/core'; +import type { ChartDataResponseResult, QueryFormData } from '@superset-ui/core'; +import { setControlValue } from 'src/explore/actions/exploreActions'; +import { getChartDataRequest } from 'src/components/Chart/chartAction'; +import { store, RootState } from 'src/views/store'; + +const getExploreState = () => (store.getState() as RootState).explore; + +const getChartId: typeof exploreApi.getChartId = () => + getExploreState().slice?.slice_id ?? undefined; + +const getControlValues: typeof exploreApi.getControlValues = () => ({ + ...(getExploreState().form_data as Record<string, unknown>), +}); + +const getControlValue: typeof exploreApi.getControlValue = (name: string) => + (getExploreState().form_data as Record<string, unknown>)[name]; + +const setControlValues: typeof exploreApi.setControlValues = async ( + values: Record<string, unknown>, +) => { + Object.entries(values).forEach(([controlName, value]) => { + store.dispatch(setControlValue(controlName, value)); + }); +}; + +const requireFormData = (): QueryFormData => { + const { form_data } = getExploreState(); + if (!form_data?.datasource) { + throw new Error('No chart is currently loaded in Explore'); + } + return form_data; +}; + +const getQuery: typeof exploreApi.getQuery = async () => { + const formData = requireFormData(); + const response = await getChartDataRequest({ Review Comment: These requests omit the active chart's `dataMask[sliceId].ownState`, so a server-paginated table on page 3 with search or sorting returns page 1/unfiltered rows and `getQuery()` describes different SQL. Can these pass the current own state, excluding `clientView` and `metricSqlExpressions` like Explore's existing query path? ########## superset-frontend/packages/superset-core/src/dashboard/index.ts: ########## @@ -0,0 +1,196 @@ +/** + * 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 Dashboard API for Superset extensions. + * + * Exposes the dashboard currently active on the Dashboard surface (see + * `navigation.getPage() === 'dashboard'`) so extensions can identify it and + * read/apply its layout, custom CSS, and native filter values. + */ + +/** + * Gets the ID of the dashboard currently active on the Dashboard surface. + * + * @returns The current dashboard's ID, or undefined if none is active. + * + * @example + * ```typescript + * const dashboardId = dashboard.getDashboardId(); + * if (dashboardId != null) { + * console.log(`Dashboard ID: ${dashboardId}`); + * } + * ``` + */ +export declare function getDashboardId(): number | undefined; + +/** + * Gets the current dashboard's full layout tree — one entry per component + * (row, column, chart holder, tab, markdown, etc.), keyed by node ID. Each + * entry has `children`, `parents`, `type`, `id`, and `meta` (grid + * size/position and other component-specific settings). + * + * @returns A map of node ID to layout node. + * + * @example + * ```typescript + * const layout = dashboard.getLayout(); + * console.log(layout['CHART-abc123'].meta.width); + * ``` + */ +export declare function getLayout(): Record<string, unknown>; Review Comment: Agreed—the documented `layout['...'].meta.width` example cannot compile because each entry is `unknown`, forcing every extension to invent an unchecked cast. Can this export a layout-node type that matches the documented shape? ########## superset-frontend/src/core/dashboard/index.ts: ########## @@ -0,0 +1,105 @@ +/** + * 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 { dashboard as dashboardApi } from '@apache-superset/core'; +import { isNativeFilter } from '@superset-ui/core'; +import type { Divider, Filter } from '@superset-ui/core'; +import { updateComponents } from 'src/dashboard/actions/dashboardLayout'; +import { dashboardInfoChanged } from 'src/dashboard/actions/dashboardInfo'; +import { updateDataMask } from 'src/dataMask/actions'; +import { store, RootState } from 'src/views/store'; + +const getState = () => store.getState() as RootState; + +const requireDashboardId = (): number => { + const { id } = getState().dashboardInfo; + if (id == null) { + throw new Error('No dashboard is currently active'); + } + return id; +}; + +const getDashboardId: typeof dashboardApi.getDashboardId = () => + getState().dashboardInfo.id ?? undefined; + +const getLayout: typeof dashboardApi.getLayout = () => ({ + ...(getState().dashboardLayout.present as Record<string, unknown>), +}); + +const updateLayoutNode: typeof dashboardApi.updateLayoutNode = async ( + nodeId: string, + meta: Record<string, unknown>, +) => { + requireDashboardId(); + const node = getState().dashboardLayout.present[nodeId]; + if (!node) { + throw new Error(`Layout node "${nodeId}" not found`); + } + // UPDATE_COMPONENTS replaces each keyed entry wholesale (it's not a deep + // merge), so the node's other fields must be carried through alongside + // the merged meta. + store.dispatch( + updateComponents({ + [nodeId]: { ...node, meta: { ...node.meta, ...meta } }, + }) as any, + ); +}; + +const getCss: typeof dashboardApi.getCss = () => getState().dashboardInfo.css ?? ''; + +const setCss: typeof dashboardApi.setCss = async (css: string) => { + requireDashboardId(); + store.dispatch(dashboardInfoChanged({ css })); +}; + +const getFilters: typeof dashboardApi.getFilters = () => { + const { nativeFilters, dataMask } = getState(); + const filterElements = Object.values(nativeFilters.filters) as Array< + Filter | Divider + >; + return filterElements.filter(isNativeFilter).map(filter => { + const mask = dataMask[filter.id]; + return { + id: filter.id, + name: filter.name, + filterType: filter.filterType, + targets: filter.targets, + extraFormData: mask?.extraFormData, + filterState: mask?.filterState, + }; + }); +}; + +const updateFilters: typeof dashboardApi.updateFilters = async ( + updates: dashboardApi.FilterValueUpdate[], +) => { + requireDashboardId(); + updates.forEach(({ filterId, extraFormData, filterState }) => { + store.dispatch(updateDataMask(filterId, { extraFormData, filterState })); + }); Review Comment: Agreed—a `filterState`-only update clears the query-producing `extraFormData`, so charts can re-query unfiltered while the filter bar still displays a selection. Can the dispatch include only fields the caller actually supplied? -- 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]
