sadpandajoe commented on code in PR #44471:
URL: https://github.com/apache/superset/pull/44471#discussion_r4143959118
##########
superset-frontend/src/pages/DashboardBuilderV2/index.tsx:
##########
@@ -177,7 +180,25 @@ export default function DashboardBuilderV2() {
// whatever the chat agent (or any other caller of the dashboard API) did.
useDashboardRevision();
const theme = useTheme();
+ const { dashboardId } = useParams<{ dashboardId?: string }>();
+ const { addDangerToast } = useToasts();
const root = dashboard.getRoot();
+
+ useEffect(() => {
+ if (!dashboardId) return undefined;
Review Comment:
Saving before this fetch returns writes the current singleton document to
the requested dashboard, so opening B and clicking Save can overwrite it with a
blank tree or dashboard A's tree. Can the editor block saves and clear/reset
state until the requested document has loaded?
##########
superset/dashboard_v2/document.py:
##########
@@ -0,0 +1,113 @@
+# 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.
+"""
+Pure helpers for the persisted Dashboard v2 document.
+
+The whole document — every node with its props inline — lives under
+``json_metadata["v2_document"]``. A node with a ``children`` array is a
+container; every other node is a leaf widget whose ``type`` names a registered
+widget type. A block is addressed by its node id.
+"""
+
+from __future__ import annotations
+
+from dataclasses import dataclass
+from typing import Any
+
+from superset.utils import json
+
+V2_DOCUMENT_KEY = "v2_document"
+DOCUMENT_VERSION = 1
+ROOT_ID = "root"
+
+_NODE_KEYS = ("type", "layout", "children", "props", "style")
+
+
+class DocumentValidationError(ValueError):
+ """The submitted v2 document is malformed."""
+
+
+@dataclass(frozen=True)
+class LeafWidget:
+ node_id: str
+ widget_type: str
+ props: dict[str, Any]
+
+
+def is_container(node: dict[str, Any]) -> bool:
+ return isinstance(node.get("children"), list)
+
+
+def load_stored_document(json_metadata: Any) -> dict[str, Any] | None:
+ """The stored document from a dashboard's ``json_metadata``, or ``None``
+ when the dashboard is not a v2 dashboard."""
+ if not isinstance(json_metadata, str):
+ return None
+ try:
+ metadata = json.loads(json_metadata or "{}")
+ except json.JSONDecodeError:
+ return None
+ document = metadata.get(V2_DOCUMENT_KEY) if isinstance(metadata, dict)
else None
+ if not isinstance(document, dict) or not isinstance(document.get("nodes"),
dict):
+ return None
+ return document
+
+
+def normalize_document(document: Any) -> tuple[dict[str, Any],
list[LeafWidget]]:
+ """
+ Validate a submitted document and return the version to store (unknown
+ node keys dropped) plus its leaf widgets, for registry validation.
+ """
+ if not isinstance(document, dict) or not isinstance(document.get("nodes"),
dict):
+ raise DocumentValidationError("document.nodes must be an object")
+ nodes: dict[str, Any] = document["nodes"]
+ root = nodes.get(ROOT_ID)
+ if not isinstance(root, dict) or not is_container(root):
+ raise DocumentValidationError("document must contain a root container")
+
+ stored: dict[str, dict[str, Any]] = {}
+ leaves: list[LeafWidget] = []
+ for node_id, node in nodes.items():
+ if not isinstance(node, dict) or not isinstance(node.get("type"), str):
+ raise DocumentValidationError(
+ f"node {node_id!r} must be an object with a string type"
+ )
+ props = node.get("props")
+ if props is not None and not isinstance(props, dict):
+ raise DocumentValidationError(f"node {node_id!r} props must be an
object")
+ if is_container(node):
+ if unknown := [child for child in node["children"] if child not in
nodes]:
Review Comment:
This accepts a node as its own child (or any cycle), which is then persisted
and recursively rendered when the dashboard is opened. Can validation reject
cycles before saving the document?
##########
superset-frontend/src/pages/DashboardBuilderV2/DashboardHeader.tsx:
##########
@@ -262,16 +302,38 @@ export default function DashboardHeader(): ReactElement {
the four of them read as one run of controls. The rule is what
says where one pair stops and the other starts. */}
<Divider type="vertical" />
+ <Button
+ buttonStyle="secondary"
+ buttonSize="small"
+ disabled={dashboardId === undefined}
+ onClick={() => setShowEmbed(true)}
+ data-test="header-embed"
+ >
+ {t('Embed')}
+ </Button>
{/* Saving commits a version; History is the versions already
committed. One concern, read in one place — so the record sits
immediately before the button that produces what it lists, rather
than at the far side of the bar from it. */}
<Inert label={t('History')} test="header-history" reads>
{t('History')}
</Inert>
- <Inert label={t('Save')} test="header-save" reads>
+ <Button
+ buttonStyle="primary"
+ buttonSize="small"
+ loading={saving}
+ onClick={save}
+ data-test="header-save"
+ >
{t('Save')}
- </Inert>
+ </Button>
+ {dashboardId !== undefined && (
+ <DashboardEmbedModal
Review Comment:
This reuses the legacy embed modal, whose SDK route renders the legacy
dashboard rather than the V2 document. Can the V2 embed action point to a
renderer that loads the saved V2 widgets before exposing this flow?
##########
superset-frontend/packages/superset-widgets/src/embed/extensionHost.tsx:
##########
@@ -0,0 +1,361 @@
+/**
+ * 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.
+ */
+
+/**
+ * The `@apache-superset/core` an extension's frontend gets when it runs in an
+ * embedded page instead of in Superset.
+ *
+ * The bundle is the same one Superset loads: it registers its widget with
+ * `views.registerView(view, 'dashboard.widgets', Component)` and renders from
+ * a node id. Here `views` lands that registration in the widget registry, and
+ * `dashboard` is backed by the embedded widget's own bus and data client
+ * rather than the builder's node tree — so an extension widget embeds with no
+ * knowledge that it is embedded.
+ */
+// eslint-disable-next-line no-restricted-syntax
+import * as supersetCore from '@apache-superset/core';
+import { useEffect, useReducer, useRef, type ComponentType } from 'react';
+import type {
+ common,
+ dashboard as dashboardApi,
+ views as viewsApi,
+} from '@apache-superset/core';
+import { DASHBOARD_WIDGETS_LOCATION } from '@apache-superset/core/widgets';
+import { getActiveResolvedFilters } from '../activeFilters';
+import { useWidgetBus } from '../bus';
+import { useWidgetDataClient } from '../dataClient';
+import {
+ registerWidgetComponent,
+ unregisterWidgetComponent,
+} from '../registry';
+import type {
+ DataBindingSpec,
+ Disposable,
+ QueryDataResult,
+ WidgetBus,
+ WidgetComponent,
+ WidgetDataClient,
+ WidgetEvent,
+ WidgetProps,
+} from '../types';
+
+/**
+ * The widget type a bare `dataBinding` executes as. `DataBindingWidget`
+ * server-side reads nothing but `props.dataBinding`, so an extension's
+ * `fetchQueryData` runs through the same validated inline path (and the same
+ * guest dataset allowlist) as any other embedded widget, without the widget
+ * API needing a second way in.
+ */
+const QUERY_WIDGET_TYPE = 'echarts';
+
+/** Extension metadata as `GET /api/v1/extensions/<publisher>/<name>` returns
it. */
+export interface ExtensionInfo {
+ id: string;
+ publisher?: string;
+ name: string;
+ version?: string;
+ description?: string;
+ dependencies?: string[];
+ remoteEntry?: string;
+ moduleFederationName?: string;
+}
+
+class EmbeddedDisposable {
+ constructor(private readonly callOnDispose: () => void) {}
+
+ static from(...disposables: { dispose: () => unknown }[]) {
+ return new EmbeddedDisposable(() =>
+ disposables.forEach(disposable => disposable.dispose()),
+ );
+ }
+
+ dispose(): void {
+ this.callOnDispose();
+ }
+}
+
+const disposable = (callOnDispose: () => void = () => {}) =>
+ new EmbeddedDisposable(callOnDispose) as unknown as common.Disposable;
+
+const notAvailable = (api: string) => () => {
+ throw new Error(`${api} is not available to an embedded widget.`);
+};
+
+interface ExtensionInstance {
+ type: string;
+ /** What the host passed; it wins over `overrides` on every render. */
+ props: Record<string, unknown>;
+ /** Props the extension wrote with `updateProps`, for this session only. */
+ overrides: Record<string, unknown>;
+ bus: WidgetBus;
+ client: WidgetDataClient;
+ rerender: () => void;
+}
+
+const instances = new Map<string, ExtensionInstance>();
+
+/**
+ * The page services the last rendered extension widget was given.
+ * `dashboard.fetchQueryData` carries no node id, so it runs through these:
+ * every widget under one provider shares a client and a bus, and the id only
+ * decides which filters a widget excludes as its own.
+ *
+ * Deliberately not cleared when a widget unmounts. A widget fetches from an
+ * effect, and React runs a child's effects before its parent's — so anything
+ * this pointer had to be restored by the adapter would already be gone by the
+ * time the extension's own effect asks for it (strict mode makes that the
+ * normal path, not an edge case).
+ */
+let active: { id: string; bus: WidgetBus; client: WidgetDataClient } |
undefined;
+
+const buses = new Set<WidgetBus>();
+
+interface Subscription {
+ eventType: string;
+ listener: (event: WidgetEvent) => void;
+ attached: Map<WidgetBus, Disposable>;
+}
+
+const subscriptions = new Set<Subscription>();
+
+/**
+ * Widgets from one extension can sit under different providers, each with its
+ * own bus, so a listener follows every bus the extension is rendered under.
+ * Buses are kept for the life of the page — they belong to a provider, not to
+ * a widget.
+ */
+function trackBus(bus: WidgetBus): void {
+ if (buses.has(bus)) return;
+ buses.add(bus);
+ subscriptions.forEach(subscription =>
+ subscription.attached.set(
+ bus,
+ bus.on(subscription.eventType, subscription.listener),
+ ),
+ );
+}
+
+function getNode(id: string): dashboardApi.DashboardNode | undefined {
+ const instance = instances.get(id);
+ if (!instance) return undefined;
+ return {
+ id,
+ type: instance.type,
+ props: { ...instance.props, ...instance.overrides },
+ };
+}
+
+function updateProps(id: string, props: Record<string, unknown>): void {
+ const instance = instances.get(id);
+ if (!instance) return;
+ instance.overrides = { ...instance.overrides, ...props };
+ instance.rerender();
+}
+
+function fetchQueryData(binding: DataBindingSpec): Promise<QueryDataResult> {
+ if (!active) {
+ throw new Error(
+ 'dashboard.fetchQueryData is only available to a rendered widget.',
+ );
+ }
+ const { id, bus, client } = active;
+ return client.fetchData({
+ instanceId: id,
+ widget: { type: QUERY_WIDGET_TYPE, props: { dataBinding: binding } },
+ filters: getActiveResolvedFilters(bus, binding.datasetId, id),
+ });
+}
+
+const dashboard = {
+ ...supersetCore.dashboard,
+ getDashboardId: () => undefined,
+ getRoot: () => ({
+ id: 'root',
+ type: 'grid',
+ children: [...instances.keys()],
+ }),
+ getNode,
+ // The host page owns where a widget sits and what it holds; an embedded
+ // widget cannot place or resize itself.
+ addWidget: notAvailable('dashboard.addWidget'),
+ removeWidget: notAvailable('dashboard.removeWidget'),
+ moveWidget: notAvailable('dashboard.moveWidget'),
+ updateLayout: notAvailable('dashboard.updateLayout'),
+ updateProps,
+ onDidLayoutChange: () => disposable(),
+ emit: (nodeId: string, eventType: string, payload: unknown) =>
+ instances.get(nodeId)?.bus.emit(nodeId, eventType, payload),
+ getValue: (nodeId: string, eventType: string) =>
+ instances.get(nodeId)?.bus.getValue(nodeId, eventType),
+ on: (eventType: string, listener: (event: WidgetEvent) => void) => {
+ const subscription: Subscription = {
+ eventType,
+ listener,
+ attached: new Map(),
+ };
+ subscriptions.add(subscription);
+ buses.forEach(bus =>
+ subscription.attached.set(bus, bus.on(eventType, listener)),
+ );
+ return disposable(() => {
+ subscriptions.delete(subscription);
+ subscription.attached.forEach(attached => attached.dispose());
+ });
+ },
+ fetchQueryData,
+} as unknown as typeof supersetCore.dashboard;
+
+/**
+ * Renders an extension's widget view from the embedded widget's props: the
+ * instance id is the node id the view is handed, so every `dashboard.*` call
+ * it makes resolves against this instance.
+ */
+export function adaptExtensionWidget(
+ type: string,
+ View: ComponentType<{ nodeId: string }>,
+): WidgetComponent {
+ function ExtensionWidget({ instanceId, props }: WidgetProps) {
+ const bus = useWidgetBus();
+ const client = useWidgetDataClient();
+ const [, rerender] = useReducer((tick: number) => tick + 1, 0);
+ const instance = useRef<ExtensionInstance>();
+
+ // Written during render, not in an effect: the view reads its node while
+ // it renders, which happens before any effect of ours would have run.
+ instance.current = {
+ type,
+ props,
+ overrides: instances.get(instanceId)?.overrides ?? {},
+ bus,
+ client,
+ rerender,
+ };
+ instances.set(instanceId, instance.current);
+ active = { id: instanceId, bus, client };
Review Comment:
`active` is shared by every rendered extension widget, so after B renders, a
query initiated by A uses B's instance ID, filters, and potentially B's data
client. Can this context be scoped to the calling widget instead of
module-global state?
--
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]
