sadpandajoe commented on code in PR #44276: URL: https://github.com/apache/superset/pull/44276#discussion_r4153624719
########## superset-frontend/src/core/dashboard/index.ts: ########## @@ -0,0 +1,303 @@ +/** + * 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, makeApi, SupersetClient } from '@superset-ui/core'; +import type { + DataMask, + Divider, + Filter, + JsonObject, + QueryFormData, +} from '@superset-ui/core'; +import { matchPath } from 'react-router-dom'; +import { updateComponents } from 'src/dashboard/actions/dashboardLayout'; +import { dashboardInfoChanged } from 'src/dashboard/actions/dashboardInfo'; +import { applySavedFilterChanges } from 'src/dashboard/actions/nativeFilters'; +import type { SaveFilterChangesType } from 'src/dashboard/components/nativeFilters/FiltersConfigModal/types'; +import { updateDataMask } from 'src/dataMask/actions'; +import { + setChartFormData, + triggerQuery, +} from 'src/components/Chart/chartAction'; +import { invalidateChartFormDataCache } from 'src/dashboard/util/charts/getFormDataWithExtraFilters'; +import { applyDefaultFormData } from 'src/explore/store'; +import extractUrlParams from 'src/dashboard/util/extractUrlParams'; +import { RoutePaths } from 'src/views/routePaths'; +import { store, RootState } from 'src/views/store'; +import { navigation } from '../navigation'; + +const getState = () => store.getState() as RootState; + +// The `:idOrSlug` the browser's current URL routes to, read directly off +// `window.location` rather than from React Router context, matching how +// `navigation`'s own page derivation works. +const getRoutedIdOrSlug = (): string | undefined => + matchPath<{ idOrSlug: string }>(window.location.pathname, { + path: RoutePaths.DASHBOARD, + exact: false, + })?.params.idOrSlug; + +// `dashboardInfo`/`dashboardLayout`/`nativeFilters`/`charts` are retained +// across an in-SPA navigation, and even once the browser has routed to a new +// dashboard, they keep the *previous* dashboard's data until that +// dashboard's HYDRATE_DASHBOARD completes. Comparing the URL's `idOrSlug` +// against `dashboardInfo`'s own id/slug — rather than trusting the page type +// alone — closes this window: they only match again once hydration has +// actually caught up, including for a same-surface dashboard-to-dashboard +// navigation. +const isDashboardActive = (): boolean => { + if (navigation.getPage() !== 'dashboard') return false; + const idOrSlug = getRoutedIdOrSlug(); + if (idOrSlug == null) return false; + const { id, slug } = getState().dashboardInfo; + return id != null && (String(id) === idOrSlug || slug === idOrSlug); +}; + +const requireDashboardId = (): number => { + const { id } = getState().dashboardInfo; + if (!isDashboardActive() || id == null) { + throw new Error('No dashboard is currently active'); + } + return id; +}; + +const getDashboardId: typeof dashboardApi.getDashboardId = () => + isDashboardActive() ? (getState().dashboardInfo.id ?? undefined) : undefined; + +// Every dashboardLayout reducer treats a node (and its `meta`/`children`/ +// `parents`) as immutable already — an update always replaces it with a new +// object rather than mutating it in place — so freezing the copy below only +// makes that existing contract explicit, without risking a reducer's own +// future in-place update on a node returned from here. +function deepFreeze<T>(value: T): T { + if (value !== null && typeof value === 'object' && !Object.isFrozen(value)) { + Object.freeze(value); + Object.values(value as Record<string, unknown>).forEach(deepFreeze); + } + return value; +} + +const getLayout: typeof dashboardApi.getLayout = () => + isDashboardActive() + ? deepFreeze({ ...getState().dashboardLayout.present }) + : {}; + +const getActiveTabs: typeof dashboardApi.getActiveTabs = () => + isDashboardActive() ? [...(getState().dashboardState.activeTabs ?? [])] : []; + +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 = () => + isDashboardActive() ? (getState().dashboardInfo.css ?? '') : ''; + +const setCss: typeof dashboardApi.setCss = async (css: string) => { + requireDashboardId(); + store.dispatch(dashboardInfoChanged({ css })); +}; + +const getFilters: typeof dashboardApi.getFilters = () => { + if (!isDashboardActive()) return []; + 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(); + const { filters: currentFilters } = getState().nativeFilters; + updates.forEach(({ filterId }) => { + if (!currentFilters[filterId]) { + throw new Error(`Filter "${filterId}" not found on this dashboard`); + } + }); + updates.forEach(({ filterId, extraFormData, filterState }) => { + const dataMask: DataMask = {}; + if (extraFormData !== undefined) { + dataMask.extraFormData = extraFormData; + } + if (filterState !== undefined) { + dataMask.filterState = filterState; + } + store.dispatch(updateDataMask(filterId, dataMask)); + }); +}; + +const saveFilters: typeof dashboardApi.saveFilters = async ( + updates: dashboardApi.FilterConfigUpdate[], + deletedFilterIds: string[] = [], +) => { + const dashboardId = requireDashboardId(); + const { filters: currentFilters } = getState().nativeFilters; + const { dataMask: currentDataMask } = getState(); + + const modified = updates.map( + ({ filterId, name, targets, defaultDataMask }) => { + const existing = currentFilters[filterId]; + if (!existing) { + throw new Error(`Filter "${filterId}" not found on this dashboard`); + } + return { + ...existing, + ...(name !== undefined && { name }), + ...(targets !== undefined && { targets }), + ...(defaultDataMask !== undefined && { defaultDataMask }), + }; + }, + ) as SaveFilterChangesType['modified']; + + if (modified.length === 0 && deletedFilterIds.length === 0) { + return; + } + + const filterChanges: SaveFilterChangesType = { + modified, + deleted: deletedFilterIds, + reordered: [], + }; + + const putFilters = makeApi<SaveFilterChangesType, { result: Filter[] }>({ + method: 'PUT', + endpoint: `/api/v1/dashboard/${dashboardId}/filters`, + }); + const response = await putFilters(filterChanges); + applySavedFilterChanges( + store.dispatch, + filterChanges, + response.result, + currentFilters, + ); + + // applySavedFilterChanges resets each modified filter's live extraFormData/ + // filterState back to its default unless the filter is required or + // defaultToFirstItem (see updateDataMaskForFilterChanges), even for an + // update that never touched the filter's value (e.g. a rename). Restore + // whatever was live immediately before this save for any filter whose + // update didn't explicitly set a new one. + updates.forEach(({ filterId, defaultDataMask }) => { + if (defaultDataMask !== undefined) return; + const liveMask = currentDataMask[filterId]; + if (!liveMask) return; + store.dispatch( + updateDataMask(filterId, { + extraFormData: liveMask.extraFormData, + filterState: liveMask.filterState, + }), + ); + }); +}; + +// TRIGGER_QUERY synchronously sets chartStatus to 'loading'; the mounted +// Chart component then re-queries and re-renders it, landing on one of +// these terminal statuses (see chartReducer.ts). +const isTerminalChartStatus = (status: string | null | undefined): boolean => + status === 'rendered' || status === 'failed' || status === 'stopped'; Review Comment: For a virtualized chart below the fold, the query can reach `success` without mounting `ChartRenderer`, so it never reaches any of these statuses and `await refreshChart()` hangs with its store subscription still registered. Could this handle charts that cannot render immediately, and reject/clean up when navigation or removal makes completion impossible? ########## superset-frontend/packages/superset-core/src/dashboard/index.ts: ########## @@ -0,0 +1,325 @@ +/** + * 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; + +/** + * One component (row, column, chart holder, tab, markdown, etc.) in a + * dashboard's layout tree, as returned by {@link getLayout}. + */ +export interface LayoutNode { + /** IDs of this node's child components, in display order. */ + children: string[]; + + /** IDs of this node's ancestor components, root first. */ + parents?: string[]; + + /** Component type, e.g. `'CHART'`, `'ROW'`, `'TABS'`, `'MARKDOWN'`. */ + type: string; + + /** This node's own ID, e.g. `'CHART-abc123'`. */ + id: string; + + /** Grid size/position and other component-specific settings. */ + meta: { + chartId?: number; + width?: number; + height?: number; + [key: string]: unknown; + }; +} + +/** + * Gets the current dashboard's full layout tree — one entry per component + * (row, column, chart holder, tab, markdown, etc.), keyed by node ID. + * + * The returned map and every node (and its nested `meta`/`children`/ + * `parents`) are frozen: mutating them has no effect on the dashboard — + * use {@link updateLayoutNode} to apply a change instead. + * + * @returns A map of node ID to layout node. + * + * @example + * ```typescript + * const layout = dashboard.getLayout(); + * const chartNode = layout['CHART-abc123']; + * console.log(chartNode.meta.width); + * ``` + */ +export declare function getLayout(): Record<string, LayoutNode>; + +/** + * Gets the IDs of the current dashboard's currently-open `TAB` layout + * nodes — one per `TABS` container, since nested tab sets can each have + * their own open tab. Use {@link getLayout} to resolve an ID to its node + * (e.g. to read its `meta.text` label). + * + * @returns The active tab node IDs, e.g. `['TAB-abc123']`. + * + * @example + * ```typescript + * const activeTabs = dashboard.getActiveTabs(); + * ``` + */ +export declare function getActiveTabs(): string[]; + +/** + * Updates a single layout node's `meta` (e.g. grid `width`/`height`, or + * other component-specific settings) on the current dashboard. Only the + * keys passed in `meta` are changed — the node's other meta fields, and the + * rest of the layout, are left as-is. + * + * @param nodeId The layout node's ID, e.g. `'CHART-abc123'`. Use + * `getLayout()` to find node IDs. + * @param meta Meta fields to merge into the node. + * @returns Promise that resolves once the layout has been updated. + * @throws If no dashboard is active, or `nodeId` doesn't exist in the + * layout. + * + * @example + * ```typescript + * await dashboard.updateLayoutNode('CHART-abc123', { width: 6, height: 50 }); + * ``` + */ +export declare function updateLayoutNode( + nodeId: string, + meta: Record<string, unknown>, +): Promise<void>; + +/** + * Gets the current dashboard's custom CSS. + * + * @returns The current CSS string (empty string if unset). + * + * @example + * ```typescript + * const css = dashboard.getCss(); + * ``` + */ +export declare function getCss(): string; + +/** + * Sets the current dashboard's custom CSS. Applies immediately to the + * rendered page — no save required. + * + * @param css The new CSS. + * @returns Promise that resolves once the CSS has been applied. + * @throws If no dashboard is active. + * + * @example + * ```typescript + * await dashboard.setCss('.dashboard-header { background: #f5f5f5; }'); + * ``` + */ +export declare function setCss(css: string): Promise<void>; + +/** + * One filter definition update, as passed to {@link saveFilters}. + */ +export interface FilterConfigUpdate { + /** + * The filter's ID. Must be an existing filter on this dashboard — + * `saveFilters` does not create new ones. Use `getFilters()` to find + * filter IDs. + */ + filterId: string; + + /** + * The filter's new display name. + */ + name?: string; + + /** + * The filter's new target columns/charts. + */ + targets?: unknown; + + /** + * The filter's new default value — applied when the dashboard next loads + * or the filter is reset — in the same shape as a filter's + * `extraFormData`/`filterState`. + */ + defaultDataMask?: { + extraFormData?: Record<string, unknown>; + filterState?: Record<string, unknown>; + }; +} + +/** + * Persists changes to the current dashboard's native filter *definitions* — + * as opposed to {@link updateFilters}, which only changes a filter's + * currently-applied value for this browser session. Changes saved here are + * visible to every future viewer of this dashboard, the same as saving + * through the Filter Bar's "Edit filters" UI. + * + * Does not create new filters or reorder existing ones — only edits or + * deletes filters that already exist on this dashboard. + * + * @param updates The filter definitions to update. + * @param deletedFilterIds IDs of filters to delete. + * @returns Promise that resolves once the changes have been saved and + * applied to this session. + * @throws If no dashboard is active, an update references a filter ID that + * doesn't exist on this dashboard, or the save fails. + * + * @example + * ```typescript + * await dashboard.saveFilters([ + * { filterId: 'NATIVE_FILTER-abc123', name: 'Region (EMEA)' }, + * ]); + * ``` + */ +export declare function saveFilters( + updates: FilterConfigUpdate[], + deletedFilterIds?: string[], +): Promise<void>; + +/** + * Represents one of the current dashboard's native filters, along with its + * currently-applied value. + */ +export interface DashboardFilter { + /** + * The filter's ID. + */ + id: string; + + /** + * The filter's display name. + */ + name?: string; + + /** + * The filter's type, e.g. `'filter_select'`, `'filter_range'`. + */ + filterType?: string; + + /** + * The columns/charts this filter targets. + */ + targets?: unknown; + + /** + * The filter's currently-applied query-modifying payload, if set. + */ + extraFormData?: Record<string, unknown>; + + /** + * The filter's currently-applied UI state (e.g. selected values), if set. + */ + filterState?: Record<string, unknown>; +} + +/** + * Gets the current dashboard's native filters, including their + * currently-applied values. + * + * @returns The dashboard's filters. + * + * @example + * ```typescript + * const filters = dashboard.getFilters(); + * ``` + */ +export declare function getFilters(): DashboardFilter[]; + +/** + * One filter value update, as passed to {@link updateFilters}. + */ +export interface FilterValueUpdate { + /** + * The filter's ID. Use `getFilters()` to find filter IDs. + */ + filterId: string; + + /** + * The new query-modifying payload to apply. + */ + extraFormData?: Record<string, unknown>; + + /** + * The new UI state (e.g. selected values) to apply. + */ + filterState?: Record<string, unknown>; +} + +/** + * Applies new values to one or more of the current dashboard's native + * filters — the same effect as a user changing a value in the Filter Bar. + * Every chart the affected filter(s) target re-queries and re-renders. + * + * @param updates The filter value updates to apply. + * @returns Promise that resolves once the updates have been applied. + * @throws If no dashboard is active. + * + * @example + * ```typescript + * await dashboard.updateFilters([ Review Comment: This example changes only the filter bar's displayed selection: `updateFilters` forwards `filterState` without deriving `extraFormData`, so an initially empty filter still leaves chart queries unfiltered. Could the example supply the matching query payload as well? ########## superset-frontend/packages/superset-core/package.json: ########## @@ -35,10 +35,18 @@ "types": "./lib/commands/index.d.ts", "default": "./lib/commands/index.js" }, + "./dashboard": { + "types": "./lib/dashboard/index.d.ts", + "default": "./lib/dashboard/index.js" Review Comment: An extension importing `@apache-superset/core/dashboard` resolves this declaration-only module rather than the host implementation: the loader and template share only the root `@apache-superset/core` key, so `dashboard.getDashboardId()` is undefined at runtime. Could these new subpaths resolve to the host namespaces, or be restricted to type-only imports? ########## 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: The routed-ID guard now closes the pre-hydration window, but a `saveFilters()` PUT started on A still applies A's response to B if navigation and hydration finish before the await returns. Could the continuation recheck the captured dashboard identity before dispatching the saved filters (and likewise before applying a fetched chart configuration)? ########## superset-frontend/src/core/dashboard/index.ts: ########## @@ -0,0 +1,303 @@ +/** + * 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, makeApi, SupersetClient } from '@superset-ui/core'; +import type { + DataMask, + Divider, + Filter, + JsonObject, + QueryFormData, +} from '@superset-ui/core'; +import { matchPath } from 'react-router-dom'; +import { updateComponents } from 'src/dashboard/actions/dashboardLayout'; +import { dashboardInfoChanged } from 'src/dashboard/actions/dashboardInfo'; +import { applySavedFilterChanges } from 'src/dashboard/actions/nativeFilters'; +import type { SaveFilterChangesType } from 'src/dashboard/components/nativeFilters/FiltersConfigModal/types'; +import { updateDataMask } from 'src/dataMask/actions'; +import { + setChartFormData, + triggerQuery, +} from 'src/components/Chart/chartAction'; +import { invalidateChartFormDataCache } from 'src/dashboard/util/charts/getFormDataWithExtraFilters'; +import { applyDefaultFormData } from 'src/explore/store'; +import extractUrlParams from 'src/dashboard/util/extractUrlParams'; +import { RoutePaths } from 'src/views/routePaths'; +import { store, RootState } from 'src/views/store'; +import { navigation } from '../navigation'; + +const getState = () => store.getState() as RootState; + +// The `:idOrSlug` the browser's current URL routes to, read directly off +// `window.location` rather than from React Router context, matching how +// `navigation`'s own page derivation works. +const getRoutedIdOrSlug = (): string | undefined => + matchPath<{ idOrSlug: string }>(window.location.pathname, { + path: RoutePaths.DASHBOARD, + exact: false, + })?.params.idOrSlug; + +// `dashboardInfo`/`dashboardLayout`/`nativeFilters`/`charts` are retained +// across an in-SPA navigation, and even once the browser has routed to a new +// dashboard, they keep the *previous* dashboard's data until that +// dashboard's HYDRATE_DASHBOARD completes. Comparing the URL's `idOrSlug` +// against `dashboardInfo`'s own id/slug — rather than trusting the page type +// alone — closes this window: they only match again once hydration has +// actually caught up, including for a same-surface dashboard-to-dashboard +// navigation. +const isDashboardActive = (): boolean => { + if (navigation.getPage() !== 'dashboard') return false; + const idOrSlug = getRoutedIdOrSlug(); + if (idOrSlug == null) return false; + const { id, slug } = getState().dashboardInfo; + return id != null && (String(id) === idOrSlug || slug === idOrSlug); +}; + +const requireDashboardId = (): number => { + const { id } = getState().dashboardInfo; + if (!isDashboardActive() || id == null) { + throw new Error('No dashboard is currently active'); + } + return id; +}; + +const getDashboardId: typeof dashboardApi.getDashboardId = () => + isDashboardActive() ? (getState().dashboardInfo.id ?? undefined) : undefined; + +// Every dashboardLayout reducer treats a node (and its `meta`/`children`/ +// `parents`) as immutable already — an update always replaces it with a new +// object rather than mutating it in place — so freezing the copy below only +// makes that existing contract explicit, without risking a reducer's own +// future in-place update on a node returned from here. +function deepFreeze<T>(value: T): T { + if (value !== null && typeof value === 'object' && !Object.isFrozen(value)) { + Object.freeze(value); + Object.values(value as Record<string, unknown>).forEach(deepFreeze); + } + return value; +} + +const getLayout: typeof dashboardApi.getLayout = () => + isDashboardActive() + ? deepFreeze({ ...getState().dashboardLayout.present }) + : {}; + +const getActiveTabs: typeof dashboardApi.getActiveTabs = () => + isDashboardActive() ? [...(getState().dashboardState.activeTabs ?? [])] : []; + +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 = () => + isDashboardActive() ? (getState().dashboardInfo.css ?? '') : ''; + +const setCss: typeof dashboardApi.setCss = async (css: string) => { + requireDashboardId(); + store.dispatch(dashboardInfoChanged({ css })); +}; + +const getFilters: typeof dashboardApi.getFilters = () => { + if (!isDashboardActive()) return []; + 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(); + const { filters: currentFilters } = getState().nativeFilters; + updates.forEach(({ filterId }) => { + if (!currentFilters[filterId]) { + throw new Error(`Filter "${filterId}" not found on this dashboard`); + } + }); + updates.forEach(({ filterId, extraFormData, filterState }) => { + const dataMask: DataMask = {}; + if (extraFormData !== undefined) { + dataMask.extraFormData = extraFormData; + } + if (filterState !== undefined) { + dataMask.filterState = filterState; + } + store.dispatch(updateDataMask(filterId, dataMask)); + }); +}; + +const saveFilters: typeof dashboardApi.saveFilters = async ( + updates: dashboardApi.FilterConfigUpdate[], + deletedFilterIds: string[] = [], +) => { + const dashboardId = requireDashboardId(); + const { filters: currentFilters } = getState().nativeFilters; + const { dataMask: currentDataMask } = getState(); + + const modified = updates.map( + ({ filterId, name, targets, defaultDataMask }) => { + const existing = currentFilters[filterId]; + if (!existing) { + throw new Error(`Filter "${filterId}" not found on this dashboard`); + } + return { + ...existing, + ...(name !== undefined && { name }), + ...(targets !== undefined && { targets }), + ...(defaultDataMask !== undefined && { defaultDataMask }), + }; + }, + ) as SaveFilterChangesType['modified']; + + if (modified.length === 0 && deletedFilterIds.length === 0) { + return; + } + + const filterChanges: SaveFilterChangesType = { + modified, + deleted: deletedFilterIds, + reordered: [], + }; + + const putFilters = makeApi<SaveFilterChangesType, { result: Filter[] }>({ + method: 'PUT', + endpoint: `/api/v1/dashboard/${dashboardId}/filters`, + }); + const response = await putFilters(filterChanges); + applySavedFilterChanges( + store.dispatch, + filterChanges, + response.result, + currentFilters, + ); + + // applySavedFilterChanges resets each modified filter's live extraFormData/ + // filterState back to its default unless the filter is required or + // defaultToFirstItem (see updateDataMaskForFilterChanges), even for an + // update that never touched the filter's value (e.g. a rename). Restore + // whatever was live immediately before this save for any filter whose + // update didn't explicitly set a new one. + updates.forEach(({ filterId, defaultDataMask }) => { + if (defaultDataMask !== undefined) return; + const liveMask = currentDataMask[filterId]; Review Comment: A rename-only save captures the live filter value before the PUT, then restores that old snapshot after it returns. If the user changes the filter while the request is pending, this silently reverts their new selection; could the preservation snapshot be taken after the await, immediately before applying the saved changes? ########## superset-frontend/src/core/explore/index.ts: ########## @@ -0,0 +1,147 @@ +/** + * 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 { omit } from 'lodash-es'; +import { explore as exploreApi } from '@apache-superset/core'; +import type { + ChartDataResponseResult, + JsonObject, + QueryFormData, +} from '@superset-ui/core'; +import { setControlValue } from 'src/explore/actions/exploreActions'; +import { getFormDataFromControls } from 'src/explore/controlUtils'; +import { QUERY_MODE_REQUISITES } from 'src/explore/constants'; +import { requestChartDataResolved } from 'src/components/Chart/chartAction'; +import { store, RootState } from 'src/views/store'; +import { navigation } from '../navigation'; + +const getExploreState = () => (store.getState() as RootState).explore; + +// The Redux slice below is retained across an in-SPA navigation, so +// checking it alone can't tell a still-active chart from a stale one left +// over from before the user navigated to another page. +const isExploreActive = (): boolean => navigation.getPage() === 'explore'; + +const getChartId: typeof exploreApi.getChartId = () => + isExploreActive() + ? (getExploreState().slice?.slice_id ?? undefined) + : undefined; + +const getControlValues: typeof exploreApi.getControlValues = () => + isExploreActive() + ? { ...(getExploreState().form_data as Record<string, unknown>) } + : {}; + +const getControlValue: typeof exploreApi.getControlValue = (name: string) => + isExploreActive() + ? (getExploreState().form_data as Record<string, unknown>)[name] + : undefined; Review Comment: Agreed—chart-history restoration updates `controls` without updating `form_data`, so these getters can return the previous control value while `getQuery()` uses the restored one. Could the reads derive from the same current control state as the query methods? -- 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]
