You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "msyavuz (via GitHub)" <gi...@apache.org> on 2026/09/01 13:41:09 UTC
Re: [PR] feat(dashboard-v2): add dataset-aware widget editing controls [superset]
msyavuz commented on code in PR #43588:
URL: https://github.com/apache/superset/pull/43588#discussion_r3904690345
##########
superset-frontend/src/pages/DashboardBuilderV2/controlValueValidation.ts:
##########
@@ -0,0 +1,111 @@
+/**
+ * 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 one path either half of the Properties panel (the schema-driven form
+ * and the JSON editor) writes a schema-controlled widget's control values
+ * through.
+ *
+ * Both representations edit the same `node.props`, but neither may write to
+ * it directly: a candidate is merged, sent to the backend's
+ * `Widget.validate_control_values` gate — the same gate the
+ * `set_widget_control_values` MCP tool commits through, reached here via its
+ * REST wrapper rather than a second, frontend-authored copy of its rules —
+ * and only committed to the store once that gate accepts it. A rejected
+ * candidate returns its errors and touches nothing: `node.props` is read
+ * here, never written to, until validation has already succeeded.
+ */
+import { SupersetClient } from '@superset-ui/core';
+import { provider } from 'src/core/dashboard/store';
+
+export type ControlValidationError = {
+ loc: (string | number)[];
+ message: string;
+};
+
+export type CommitPropsResult =
+ | { ok: true; values: Record<string, unknown> }
+ | { ok: false; errors: ControlValidationError[] };
+
+/**
+ * SupersetClient rejects a non-2xx response with the raw, unparsed `Response`
+ * object rather than an `Error`, so a plain `String(e)` yields the useless
+ * "[object Response]". Pull the actual `{message}`/`{errors:[...]}` body
+ * Superset sends back (same shape `chartData.ts` handles).
+ */
+export async function describeError(e: unknown): Promise<string> {
+ if (typeof Response !== 'undefined' && e instanceof Response) {
+ try {
+ const body = await e.clone().json();
+ const detail =
+ body?.message ??
+ (Array.isArray(body?.errors)
+ ? body.errors
+ .map((err: { message?: string }) => err.message)
+ .join('; ')
+ : undefined);
+ return detail
+ ? `${e.status} ${e.statusText}: ${detail}`
+ : `${e.status} ${e.statusText}`;
+ } catch {
+ return `${e.status} ${e.statusText}`;
+ }
+ }
+ return e instanceof Error ? e.message : String(e);
+}
+
+async function validateControlValues(
+ widgetType: string,
+ controlValues: Record<string, unknown>,
+): Promise<ControlValidationError[]> {
+ const { json } = await SupersetClient.post({
+ endpoint: `/api/v1/widgets/type/${widgetType}/validate`,
+ jsonPayload: { control_values: controlValues },
+ });
+ return (json as { result: { errors: ControlValidationError[] } }).result
+ .errors;
+}
+
+/**
+ * Merges `delta` onto the node's current props, validates the merged
+ * candidate, and commits it to `node.props` only if the backend accepts it.
+ *
+ * `onBeforeCommit`, if given, runs synchronously immediately before the
+ * `provider.updateProps` call — not part of the merge/validate/commit
+ * contract itself, but the one hook a caller needs to set a flag in the
+ * exact tick its own commit lands (e.g. `SchemaControlPanel` telling its
+ * own resync effect "this `props` change was mine"), without racing the
+ * async validation round-trip to do it.
+ */
+export async function commitWidgetProps(
+ nodeId: string,
+ widgetType: string,
+ delta: Record<string, unknown>,
+ options?: { onBeforeCommit?: () => void },
+): Promise<CommitPropsResult> {
+ const node = provider.getNode(nodeId);
+ const candidate = { ...node?.props, ...delta };
+ const errors = await validateControlValues(widgetType, candidate);
+ if (errors.length > 0) {
+ return { ok: false, errors };
+ }
+ options?.onBeforeCommit?.();
+ provider.updateProps(nodeId, candidate);
Review Comment:
Seconding the bot on this one — `updateProps` runs inside the awaited commit, so the caller's `validateSeqRef` guard can't stop a superseded edit, and two quick edits where the first validates slower will land the stale candidate last.
##########
superset-frontend/src/pages/DashboardBuilderV2/SchemaControlPanel.tsx:
##########
@@ -354,13 +381,42 @@ export default function SchemaControlPanel({ nodeId }: { nodeId: string }) {
}
}
- // Our own edit — don't let the resync effect remount the form
- // for it (that would drop input focus mid-typing).
- selfEditRef.current = true;
- provider.updateProps(nodeId, next);
- maybeRefreshSchema(next);
+ validateSeqRef.current += 1;
+ const seq = validateSeqRef.current;
+ commitWidgetProps(nodeId, widgetType, next, {
+ // Only fires once validation has already accepted the
+ // candidate, immediately before the commit that changes
+ // `propsKey` — so the resync effect never remounts the form
+ // and drops input focus for an edit it made itself.
+ onBeforeCommit: () => {
+ selfEditRef.current = true;
+ },
+ })
+ .then(result => {
+ if (validateSeqRef.current !== seq) return;
+ setValidationErrors(result.ok ? [] : result.errors);
Review Comment:
On rejection JsonForms keeps the typed value but `props` doesn't, and `config.formData` (line 344) is the committed props — so picking a dataset that fails validation shows it in the select while the column/metric pickers below still resolve no `datasetId`.
##########
superset-frontend/src/pages/DashboardBuilderV2/schemaControlRenderers.tsx:
##########
@@ -93,24 +164,1114 @@ function ColorControl({
path,
schema,
label,
+ description,
}: ControlProps) {
const theme = useTheme();
const value = (data ??
(schema as Record<string, unknown>).default ??
theme.colorText) as string;
return (
- <Flex align="center" gap="small">
+ <Form.Item label={label} tooltip={description}>
<input
type="color"
Review Comment:
Raw `<input type="color">` here, while `SeriesEntryPropertyControl` at line 261 uses `ColorPicker` from `@superset-ui/core/components` for the same job — the native swatch ignores the theme.
##########
superset/core/api/core_api_injection.py:
##########
@@ -328,6 +328,14 @@ def decorator(cls: Any) -> Any:
cls.name = name
cls.description = description or ""
registry[key] = cls
+ try:
+ # Validate the static schema and dependency graph at import
+ # time. Runtime enrichers may perform permission-scoped data
+ # lookups, so they must wait until a request user exists.
+ cls.validate_control_schema()
+ except Exception:
+ registry.pop(key, None)
+ raise
Review Comment:
This also runs for extension-registered widgets (line 311) — does one third-party widget with an unbuildable schema now fail the whole extension import rather than just being rejected?
##########
superset-frontend/src/core/dashboard/widgets/echartsStructuredChrome.ts:
##########
@@ -0,0 +1,144 @@
+/**
+ * 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 `echarts` widget's second, independent structured layer — chart
+ * chrome (title/legend/tooltip/axis labels) — matching `EchartsChrome`'s
+ * docstring in `superset/widgets/controls.py`: every field is optional and
+ * applies (or not) on its own, regardless of `chartType`/`customize`. A
+ * field left at its default never touches `echartsOptions`; when it does
+ * apply, it merges onto — rather than replaces — the matching section, so
+ * an unmanaged sibling property there (e.g. a hand-authored `legend.orient`)
+ * survives.
+ *
+ * `EchartsChromeValue` is deliberately flat, not grouped into
+ * `title`/`legend`/`tooltip`/`xAxis`/`yAxis` sub-objects — see
+ * `EchartsChrome`'s own docstring: JsonForms' generated control panel only
+ * renders one level of nested-object properties, so a two-level-deep
+ * `chrome.title.text` would render as an empty group with no fields inside.
+ */
+
+export interface EchartsChromeValue {
+ titleText?: string;
+ legendShow?: boolean;
+ legendPosition?: 'top' | 'bottom' | 'left' | 'right' | null;
+ tooltipTrigger?: 'item' | 'axis' | null;
+ xAxisName?: string;
+ xAxisRotate?: number;
+ xAxisFormat?: string;
+ yAxisName?: string;
+ yAxisRotate?: number;
+ yAxisFormat?: string;
+}
+
+// ECharts has no single "position" property on `legend` — placement comes
+// from `top`/`left` (each accepting a keyword or coordinate). This maps the
+// friendlier compass-direction picker onto the pair ECharts actually reads.
+const LEGEND_POSITION: Record<string, { top: string; left: string }> = {
+ top: { top: 'top', left: 'center' },
+ bottom: { top: 'bottom', left: 'center' },
+ left: { top: 'middle', left: 'left' },
+ right: { top: 'middle', left: 'right' },
+};
+
+function asRecord(value: unknown): Record<string, unknown> {
+ return value !== null && typeof value === 'object'
+ ? (value as Record<string, unknown>)
+ : {};
+}
+
+function applyTitle(existing: unknown, chrome: EchartsChromeValue): unknown {
Review Comment:
`applyTitle`'s merge is always discarded — `ChartWidget.tsx:359-360` deletes `option.title` right after `applyStructuredChrome` returns, and that's its only production caller.
##########
superset-core/src/superset_core/widgets/composites.py:
##########
@@ -0,0 +1,106 @@
+# 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.
+
+"""
+Reusable, discoverable building blocks for Dashboard V2 widget control models.
+
+A composite control is a ``BaseModel`` mixin carrying one or more fields
+(with their ``x-control`` schema extras already set) that a widget's
+``controls_class`` composes in via plain single inheritance — no nesting, so
+the composed field renders exactly where a directly-declared field would.
+The ``@composite_control`` decorator only registers the class for discovery
+(docs generation, MCP tooling); composing one into a widget never touches the
+registry.
+
+Composing more than one composite control into the same model (multiple
+inheritance across two or more registered mixins) is unsupported for now —
+see the design spec's Scope section.
+"""
+
+from __future__ import annotations
+
+from dataclasses import dataclass
+from types import MappingProxyType
+from typing import Any, Callable, Mapping
+
+from pydantic import BaseModel, ConfigDict, Field
+
+
+@dataclass(frozen=True)
+class CompositeControlInfo:
+ name: str
+ title: str
+ description: str
+ model: type[BaseModel]
+
+
+_registry: dict[str, CompositeControlInfo] = {}
+
+
+def composite_control(
+ name: str, title: str, description: str
+) -> Callable[[type[BaseModel]], type[BaseModel]]:
+ """Register a reusable Pydantic mixin as a discoverable composite control.
+
+ Composing one into a widget's ``controls_class`` is plain inheritance —
+ this decorator only makes the class discoverable via
+ ``list_composite_controls()``.
+ """
+
+ def decorator(cls: type[BaseModel]) -> type[BaseModel]:
+ if name in _registry:
+ raise ValueError(f"composite control {name!r} already registered")
+ _registry[name] = CompositeControlInfo(name, title, description, cls)
+ return cls
+
+ return decorator
+
+
+def list_composite_controls() -> Mapping[str, CompositeControlInfo]:
+ """Read-only view of registered composite controls, for docs/MCP
+ discovery. An extension-defined composite appears only once its defining
+ module has been imported (decorator side effect, same as ``@widget``)."""
+ return MappingProxyType(_registry)
+
+
+@composite_control(
+ name="metric",
+ title="Metrics",
+ description=(
+ "Reusable metric-list field (saved-metric names or ad-hoc SIMPLE aggregates)."
+ ),
+)
+class MetricControl(BaseModel):
+ """Mixin providing a ``metrics`` field, extracted verbatim from
+ ``DataBinding`` for reuse outside it."""
+
+ model_config = ConfigDict(populate_by_name=True)
+
+ metrics: list[Any] = Field(
+ title="Metrics",
+ description=(
+ "Metrics to fetch. Each entry is EITHER a string naming a saved "
+ 'metric on the dataset (e.g. "count"), OR an ad-hoc aggregate '
+ "object of the shape "
+ '{"expressionType": "SIMPLE", "column": {"column_name": "<col>"}, '
Review Comment:
The new Custom SQL tab emits `{expressionType: "SQL", sqlExpression}` via `toPlainMetric` — should this description still say SIMPLE-only and "do not pass a raw SQL string"?
##########
superset-frontend/src/pages/DashboardBuilderV2/AdhocMetricEditor.tsx:
##########
@@ -0,0 +1,263 @@
+/**
+ * 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.
+ */
+
+/**
+ * A popover editor for one ad-hoc metric — Dashboard V2's own, not the
+ * classic Explore control panel's `AdhocMetricEditPopover`. That component
+ * is a fine model for the *shape* of an ad-hoc metric, but not for reuse
+ * verbatim here: it reads `state.explore.compatibleMetrics` via
+ * `useSelector`, which throws outside a react-redux `Provider`, and Dashboard
+ * V2 is deliberately non-Redux; its own "Saved" tab is also redundant with
+ * the picker `MetricMultiControl` already offers for saved metrics, so only
+ * the Simple and Custom SQL tabs represent capability worth reusing.
+ *
+ * What *is* reused: the legacy `AdhocMetric` class (`MetricControl/AdhocMetric.ts`)
+ * as the editing/validation model — the same `expressionType`/`column`/
+ * `aggregate`/`sqlExpression`/`label` shape the backend and the classic
+ * control panel both already agree on, so nothing about "what an ad-hoc
+ * metric is" is redefined here — plus `AGGREGATES_OPTIONS` and
+ * `SQLEditorWithValidation`, both already free of Explore-specific coupling.
+ */
+import { useEffect, useState } from 'react';
+import type { ReactElement, ReactNode } from 'react';
+import { t } from '@apache-superset/core/translation';
+import { useTheme } from '@apache-superset/core/theme';
+import type { AdhocMetric as CoreAdhocMetric } from '@superset-ui/core';
+import {
+ Button,
+ Form,
+ Popover,
+ Select,
+ Tabs,
+} from '@superset-ui/core/components';
+import LegacyAdhocMetric, {
+ EXPRESSION_TYPES,
+ fromCoreAdhocMetric,
+} from 'src/explore/components/controls/MetricControl/AdhocMetric';
+import { AGGREGATES_OPTIONS } from 'src/explore/constants';
+import SQLEditorWithValidation from 'src/components/SQLEditorWithValidation';
+import type { DatasetColumnMeta } from 'src/core/dashboard/datasetMetadata';
+
+const BLANK_METRIC = { expressionType: EXPRESSION_TYPES.SIMPLE };
+
+/** The plain, storable shape `handleChange` writes back into `node.props` —
+ * only the fields `@superset-ui/core`'s `AdhocMetric` type declares, not the
+ * legacy class's extra runtime-only fields (`datasourceWarning` and the
+ * like). */
+function toPlainMetric(draft: LegacyAdhocMetric): CoreAdhocMetric {
+ const base = {
+ label: draft.label,
+ hasCustomLabel: draft.hasCustomLabel,
+ optionName: draft.optionName,
+ };
+ if (draft.expressionType === EXPRESSION_TYPES.SQL) {
+ return {
+ ...base,
+ expressionType: 'SQL',
+ sqlExpression: draft.sqlExpression ?? '',
+ } as CoreAdhocMetric;
+ }
+ return {
+ ...base,
+ expressionType: 'SIMPLE',
+ column: draft.column!,
+ aggregate: draft.aggregate,
+ } as CoreAdhocMetric;
+}
+
+export interface AdhocMetricEditorProps {
+ /** `undefined` opens the editor on a blank draft (adding a new metric). */
+ value: CoreAdhocMetric | undefined;
+ columns: DatasetColumnMeta[];
+ datasourceId: number | undefined;
+ datasourceType: string | undefined;
+ open: boolean;
+ onOpenChange: (open: boolean) => void;
+ onSave: (metric: CoreAdhocMetric) => void;
+ children: ReactNode;
+}
+
+/**
+ * Wraps `children` (the row's own clickable label) in a click-triggered
+ * popover editing one ad-hoc metric. Nothing commits until Save — Cancel, an
+ * outside click, or Escape all discard the draft, matching every other
+ * validated-commit surface in this panel where a candidate is built and
+ * discarded freely before it's actually written.
+ */
+export default function AdhocMetricEditor({
Review Comment:
No test file for this one — `schemaControlRenderers.test.tsx` only covers the exported pure helpers, so the SIMPLE/SQL round-trip through `toPlainMetric` and the `isValid()` save gate are untested.
##########
superset-frontend/src/pages/DashboardBuilderV2/schemaControlRenderers.tsx:
##########
@@ -93,24 +164,1114 @@ function ColorControl({
path,
schema,
label,
+ description,
}: ControlProps) {
const theme = useTheme();
const value = (data ??
(schema as Record<string, unknown>).default ??
theme.colorText) as string;
return (
- <Flex align="center" gap="small">
+ <Form.Item label={label} tooltip={description}>
<input
type="color"
value={value}
onChange={event => handleChange(path, event.target.value)}
/>
- <Typography.Text>{label}</Typography.Text>
- </Flex>
+ </Form.Item>
+ );
+}
+
+interface SeriesEntryPropertySchema {
+ type?: string;
+ title?: string;
+ default?: unknown;
+ minimum?: number;
+ maximum?: number;
+ 'x-control'?: string;
+ 'x-step'?: number;
+}
+
+interface SeriesEntrySchema {
+ properties?: Record<string, SeriesEntryPropertySchema>;
+}
+
+interface SeriesMapSchema {
+ properties?: Record<string, SeriesEntrySchema>;
+}
+
+type SeriesOverrideValue = Record<string, unknown>;
+
+/** Turns a camelCase field key into the lowercase, space-separated phrase
+ * an aria-label reads naturally with (`sizeScale` → `size scale`). */
+function humanizeFieldKey(key: string): string {
+ return key
+ .replace(/([A-Z])/g, ' $1')
+ .toLowerCase()
+ .trim();
+}
+
+/** The value a not-yet-customized series entry starts from, read purely off
+ * its own enriched sub-schema — every widget's per-series entry shape
+ * (Balloons' `{color, sizeScale}`, echarts' `{color, visible, displayName}`,
+ * or any future one) declares its own field defaults; this has no
+ * hard-coded opinion of what those fields are. `fallbackColor` only backs a
+ * `color` field the backend never defaulted — not expected to happen, but a
+ * theme token beats a literal black. */
+export function seriesDefaults(
+ entrySchema: SeriesEntrySchema | undefined,
+ fallbackColor: string,
+): SeriesOverrideValue {
+ const properties = entrySchema?.properties ?? {};
+ const value: SeriesOverrideValue = Object.fromEntries(
+ Object.entries(properties).map(([key, prop]) => [key, prop.default]),
+ );
+ if (value.color === undefined && 'color' in properties) {
+ value.color = fallbackColor;
+ }
+ return value;
+}
+
+/** One property of a customized series entry, rendered by its schema shape
+ * — not by a hard-coded field name — so a new per-series entry model (e.g.
+ * echarts' `{color, visible, displayName}`) needs no renderer of its own:
+ * - `x-control: "color"` (or the property key `color`) → a color swatch.
+ * - `type: "boolean"` → a toggle.
+ * - `type: "number"` / `"integer"` → a bounded numeric input.
+ * - anything else → a text input.
+ */
+function SeriesEntryPropertyControl({
+ seriesKey,
+ propKey,
+ propSchema,
+ value,
+ onChange,
+}: {
+ seriesKey: string;
+ propKey: string;
+ propSchema: SeriesEntryPropertySchema | undefined;
+ value: unknown;
+ onChange: (next: unknown) => void;
+}): ReactElement {
+ const theme = useTheme();
+ const fieldLabel = `${seriesKey} ${humanizeFieldKey(propKey)}`;
+
+ if (propSchema?.['x-control'] === 'color' || propKey === 'color') {
+ const color = (value as string) || theme.colorText;
+ return (
+ <ColorPicker
+ value={color}
+ onChange={next => onChange(next.toHexString())}
+ >
+ <button
+ type="button"
+ aria-label={t('%s color', seriesKey)}
+ style={{
+ width: 20,
+ height: 20,
+ borderRadius: 4,
+ border: '1px solid rgba(0, 0, 0, 0.15)',
+ background: color,
+ cursor: 'pointer',
+ padding: 0,
+ }}
+ />
+ </ColorPicker>
+ );
+ }
+ if (propSchema?.type === 'boolean') {
+ return (
+ <Switch
+ aria-label={fieldLabel}
+ checked={value !== false}
+ onChange={onChange}
+ />
+ );
+ }
+ if (propSchema?.type === 'number' || propSchema?.type === 'integer') {
+ return (
+ <InputNumber
+ size="small"
+ aria-label={fieldLabel}
+ value={value as number}
+ min={propSchema?.minimum}
+ max={propSchema?.maximum}
+ step={propSchema?.['x-step'] ?? 1}
+ style={{ width: 64 }}
+ onChange={next => onChange(typeof next === 'number' ? next : value)}
+ />
+ );
+ }
+ return (
+ <Input
+ size="small"
+ aria-label={fieldLabel}
+ value={(value as string) ?? ''}
+ onChange={event => onChange(event.target.value)}
+ style={{ width: 140 }}
+ />
+ );
+}
+
+/**
+ * `x-dynamic: true` on a dict-of-objects field (e.g. Balloons'
+ * `customize.series`, one entry per distinct color-dimension value, or
+ * echarts' `customize.series`, one entry per `dataBinding` metric): an
+ * overrides list, collapsed by default, rather than the upstream renderer's
+ * one always-expanded group per possible entry — which turns a real
+ * grouping column into thousands of pixels of identical, unstyled controls.
+ *
+ * The backend enriches this field's schema with one inlined per-key
+ * sub-schema (a title and, where the entry shape has one, a palette-defaulted
+ * `color`), but leaves the *data* untouched until an author actually edits a
+ * value — so "has an entry in `data`" already means "has been customized",
+ * with no comparison against the schema's own defaults needed. Which fields
+ * an entry has, and how each renders, comes entirely from that per-key
+ * sub-schema (see `SeriesEntryPropertyControl`) — this component has no
+ * opinion of its own on the entry shape.
+ */
+function SeriesOverridesControl(props: ControlProps): ReactElement {
+ const { data, handleChange, path, schema, label, description } = props;
+ const theme = useTheme();
+ const seriesSchema = schema as SeriesMapSchema;
+ const keys = useMemo(
+ () => Object.keys(seriesSchema.properties ?? {}),
+ [seriesSchema],
+ );
+ const values = (data ?? {}) as Record<string, SeriesOverrideValue>;
+ const customizedKeys = keys.filter(key => values[key] !== undefined);
+ const availableKeys = keys.filter(key => values[key] === undefined);
+
+ if (keys.length === 0) {
+ return (
+ <Form.Item label={label} tooltip={description}>
+ <Typography.Text type="secondary">
+ {t('No series available to customize yet.')}
+ </Typography.Text>
+ </Form.Item>
+ );
+ }
+
+ const write = (next: Record<string, SeriesOverrideValue>) =>
+ handleChange(path, next);
+
+ const removeOverride = (key: string) => {
+ const next = { ...values };
+ delete next[key];
+ write(next);
+ };
+
+ const addOverride = (key: string) => {
+ write({
+ ...values,
+ [key]: seriesDefaults(seriesSchema.properties?.[key], theme.colorText),
+ });
+ };
+
+ return (
+ <Form.Item label={label} tooltip={description}>
+ <Collapse
+ ghost
+ size="small"
+ items={[
+ {
+ key: 'series-overrides',
+ label: t(
+ '%s series · %s customized',
+ keys.length,
+ customizedKeys.length,
+ ),
+ children: (
+ <Flex vertical gap="small">
+ {customizedKeys.map(key => {
+ const value = values[key];
+ const entryProperties =
+ seriesSchema.properties?.[key]?.properties ?? {};
+ const otherPropKeys = Object.keys(entryProperties).filter(
+ propKey => propKey !== 'color',
+ );
+ return (
+ <Flex key={key} align="center" gap="small">
+ {'color' in entryProperties && (
+ <SeriesEntryPropertyControl
+ seriesKey={key}
+ propKey="color"
+ propSchema={entryProperties.color}
+ value={value.color}
+ onChange={next =>
+ write({
+ ...values,
+ [key]: { ...value, color: next },
+ })
+ }
+ />
+ )}
+ <div
+ style={{
+ flex: 1,
+ minWidth: 0,
+ whiteSpace: 'nowrap',
+ overflow: 'hidden',
+ textOverflow: 'ellipsis',
+ }}
+ >
+ {key}
+ </div>
+ {otherPropKeys.map(propKey => (
+ <SeriesEntryPropertyControl
+ key={propKey}
+ seriesKey={key}
+ propKey={propKey}
+ propSchema={entryProperties[propKey]}
+ value={value[propKey]}
+ onChange={next =>
+ write({
+ ...values,
+ [key]: { ...value, [propKey]: next },
+ })
+ }
+ />
+ ))}
+ <Button
+ buttonSize="xsmall"
+ buttonStyle="link"
+ aria-label={t('Reset %s to default', key)}
+ icon={<Icons.CloseOutlined iconSize="s" />}
+ onClick={() => removeOverride(key)}
+ />
+ </Flex>
+ );
+ })}
+ {availableKeys.length > 0 && (
+ // Remounted on every pick: rc-select otherwise keeps
+ // showing the just-picked option's label internally even
+ // once it's gone from `options` (a controlled `value` of
+ // `null`/`undefined` doesn't clear that cache on its own —
+ // see `ReferenceMultiList`'s identical, pre-existing gap).
+ <Select
+ key={availableKeys.length}
+ value={null}
+ placeholder={t('Add a series override…')}
+ ariaLabel={t('Add %s override', label)}
+ options={availableKeys.map(key => ({
+ value: key,
+ label: key,
+ }))}
+ onChange={next => addOverride(next as string)}
+ css={{ width: '100%' }}
+ />
+ )}
+ </Flex>
+ ),
+ },
+ ]}
+ />
+ </Form.Item>
+ );
+}
+
+/**
+ * The dataset id a column/metric-reference control resolves options
+ * against. A JsonForms control only sees its own field's data by default —
+ * the whole node's props reach it through `config.formData`, which
+ * `SchemaControlPanel` populates for exactly this reason (mirroring
+ * `SemanticLayerModal`'s own `config={{ formData }}`).
+ */
+function useBoundDatasetId(props: ControlProps): number | undefined {
+ const formData = (
+ props.config as { formData?: Record<string, unknown> } | undefined
+ )?.formData;
+ const dataBinding = formData?.dataBinding as
+ { datasetId?: number } | undefined;
+ return dataBinding?.datasetId;
+}
+
+/** The sibling `dataBinding.dimensions` list, read the same way. */
+function useBoundDimensions(props: ControlProps): string[] {
+ const formData = (
+ props.config as { formData?: Record<string, unknown> } | undefined
+ )?.formData;
+ const dataBinding = formData?.dataBinding as
+ { dimensions?: string[] } | undefined;
+ return dataBinding?.dimensions ?? [];
+}
+
+/**
+ * True when a column/metric-reference control should fail open to the raw
+ * JSON editor (`CodeControl`) rather than render its picker: no dataset is
+ * bound yet, or the dataset fetch failed. Deliberately does NOT cover the
+ * in-flight loading state (`metadata` still `null`, no `error` yet) — that's
+ * the normal case while a bound dataset's metadata is fetched, and the
+ * picker renders as usual with its own `loading` flag set.
+ */
+function shouldFallBackToCode(
+ datasetId: number | undefined,
+ error: string | null,
+): boolean {
+ return datasetId === undefined || error !== null;
+}
+
+interface ReferenceOption {
+ value: string;
+ label: ReactNode;
+}
+
+const COLUMN_TYPE_BY_HINT: Record<string, number> = {
+ numeric: 0,
+ string: 1,
+ temporal: 2,
+ boolean: 3,
+};
+
+/**
+ * Column options for a `column`/`column-multi` control, filtered by the
+ * field's `x-column-types` hint (omitted means any column type).
+ */
+export function columnOptions(
+ metadata: DatasetMetadata | null,
+ allowedTypes: string[] | undefined,
+): ReferenceOption[] {
+ const allowed = allowedTypes?.map(hint => COLUMN_TYPE_BY_HINT[hint]);
+ return (metadata?.columns ?? [])
+ .filter(
+ column =>
+ !allowed || (column.type !== null && allowed.includes(column.type)),
+ )
+ .map(column => ({
+ value: column.name,
+ label: (
+ <Flex align="center" gap="small">
+ <ColumnTypeLabel type={column.type ?? undefined} />
+ {column.name}
+ </Flex>
+ ),
+ }));
+}
+
+/** A single reference value (column or metric), rendered as a Select. */
+function ReferenceSelect({
+ label,
+ description,
+ value,
+ options,
+ loading,
+ disabled,
+ placeholder,
+ onChange,
+}: {
+ label: string;
+ description: string | undefined;
+ value: string | undefined;
+ options: ReferenceOption[];
+ loading: boolean;
+ disabled: boolean;
+ placeholder?: string;
+ onChange: (next: string | undefined) => void;
+}): ReactElement {
+ return (
+ <Form.Item label={label} tooltip={description}>
+ <Select
+ ariaLabel={label}
+ value={value}
+ onChange={next => onChange((next as string | undefined) ?? undefined)}
+ options={options}
+ loading={loading}
+ disabled={disabled}
+ placeholder={placeholder}
+ allowClear
+ css={{ width: '100%' }}
+ />
+ </Form.Item>
+ );
+}
+
+/**
+ * An ordered list of reference values (columns or metrics): each entry can
+ * be removed or dragged to reorder, and a trailing Select adds one more from
+ * whatever isn't already picked.
+ */
+function ReferenceMultiList({
+ label,
+ description,
+ values,
+ options,
+ loading,
+ disabled,
+ onChange,
+}: {
+ label: string;
+ description: string | undefined;
+ values: string[];
+ options: ReferenceOption[];
+ loading: boolean;
+ disabled: boolean;
+ onChange: (next: string[]) => void;
+}): ReactElement {
+ const dragIndexRef = useRef<number | null>(null);
+ const available = options.filter(option => !values.includes(option.value));
+
+ const move = (from: number, to: number) => {
+ const next = [...values];
+ const [moved] = next.splice(from, 1);
+ next.splice(to, 0, moved);
+ onChange(next);
+ };
+
+ return (
+ <Form.Item label={label} tooltip={description}>
+ <Flex vertical gap="small">
+ {values.map((value, index) => {
+ const option = options.find(candidate => candidate.value === value);
+ return (
+ <Flex
+ key={value}
+ align="center"
+ gap="small"
+ draggable
+ onDragStart={() => {
+ dragIndexRef.current = index;
+ }}
+ onDragOver={event => event.preventDefault()}
+ onDrop={() => {
+ if (
+ dragIndexRef.current !== null &&
+ dragIndexRef.current !== index
+ ) {
+ move(dragIndexRef.current, index);
+ }
+ dragIndexRef.current = null;
+ }}
+ >
+ <Icons.HolderOutlined iconSize="s" />
+ <div style={{ flex: 1 }}>{option?.label ?? value}</div>
+ {/* Keyboard-operable equivalent of the drag handle above: that
+ handle isn't itself focusable, so reordering — which
+ decides e.g. the default color dimension — had no
+ non-mouse path at all. */}
+ <Button
+ buttonSize="xsmall"
+ buttonStyle="link"
+ aria-label={t('Move %s up', value)}
+ disabled={index === 0}
+ icon={<Icons.UpOutlined iconSize="s" />}
+ onClick={() => move(index, index - 1)}
+ />
+ <Button
+ buttonSize="xsmall"
+ buttonStyle="link"
+ aria-label={t('Move %s down', value)}
+ disabled={index === values.length - 1}
+ icon={<Icons.DownOutlined iconSize="s" />}
+ onClick={() => move(index, index + 1)}
+ />
+ <Button
+ buttonSize="xsmall"
+ buttonStyle="link"
+ aria-label={t('Remove %s', value)}
+ icon={<Icons.CloseOutlined iconSize="s" />}
+ onClick={() => onChange(values.filter((_, i) => i !== index))}
+ />
+ </Flex>
+ );
+ })}
+ {available.length > 0 && (
+ // Remounted on every pick — see `SeriesOverridesControl`'s
+ // identical picker for why a controlled `value` of `undefined`
+ // alone doesn't stop rc-select echoing the just-picked label.
+ <Select
+ key={available.length}
+ value={null}
+ placeholder={t('Add field')}
+ ariaLabel={t('Add %s', label)}
+ options={available}
+ loading={loading}
+ disabled={disabled}
+ onChange={next => onChange([...values, next as string])}
+ css={{ width: '100%' }}
+ />
+ )}
+ </Flex>
+ </Form.Item>
+ );
+}
+
+/**
+ * `x-control: "column"` — a single column reference. Falls back to the raw
+ * JSON editor when no dataset is bound (or its fetch failed), or when the
+ * existing value isn't a string — e.g. an object hand-authored into the
+ * field through the Inspector's JSON tab, which `Select` can't render as a
+ * `value` and JsonForms would otherwise crash on.
+ */
+function ColumnControl(props: ControlProps): ReactElement {
+ const datasetId = useBoundDatasetId(props);
+ const { metadata, loading, error } = useDatasetMetadata(datasetId);
+ const allowedTypes = (props.schema as Record<string, unknown>)[
+ 'x-column-types'
+ ] as string[] | undefined;
+
+ if (
+ shouldFallBackToCode(datasetId, error) ||
+ (props.data !== undefined && typeof props.data !== 'string')
+ ) {
+ return <CodeControl {...props} />;
+ }
+
+ return (
+ <ReferenceSelect
+ label={props.label}
+ description={props.description}
+ value={props.data as string | undefined}
+ options={columnOptions(metadata, allowedTypes)}
+ loading={loading}
+ disabled={!props.enabled}
+ onChange={value => props.handleChange(props.path, value)}
+ />
+ );
+}
+
+/**
+ * The `colorDimension` field specifically: a column reference, but not to
+ * any column — the widget only colors by a dimension it already groups by
+ * (Balloons' `_color_dimension_must_be_grouped` validator rejects anything
+ * else). Offering all of a dataset's columns, most of which the backend
+ * will reject, taught nothing about which one was actually valid; this
+ * intersects the picker's options with the sibling `dataBinding.dimensions`
+ * instead, and disables it with an explanatory placeholder when there's
+ * nothing grouped yet to color by.
+ */
+function ColorDimensionControl(props: ControlProps): ReactElement {
+ const datasetId = useBoundDatasetId(props);
+ const dimensions = useBoundDimensions(props);
+ const { metadata, loading, error } = useDatasetMetadata(datasetId);
+
+ if (
+ shouldFallBackToCode(datasetId, error) ||
+ (props.data !== undefined && typeof props.data !== 'string')
+ ) {
+ return <CodeControl {...props} />;
+ }
+
+ const options = columnOptions(metadata, undefined).filter(option =>
+ dimensions.includes(option.value),
+ );
+
+ return (
+ <ReferenceSelect
+ label={props.label}
+ description={props.description}
+ value={props.data as string | undefined}
+ options={options}
+ loading={loading}
+ disabled={!props.enabled || dimensions.length === 0}
+ placeholder={
+ dimensions.length === 0 ? t('Group a dimension first') : undefined
+ }
+ onChange={value => props.handleChange(props.path, value)}
+ />
);
}
-/** Base Semantic-Layer renderers plus the widget-control code/color ones. */
+/**
+ * `x-control: "column-multi"` — an ordered list of column references. Falls
+ * back to the raw JSON editor when no dataset is bound (or its fetch
+ * failed), or when an existing entry isn't a string — e.g. an object
+ * hand-authored into the field through the Inspector's JSON tab, which
+ * `ReferenceMultiList` can't render as a list entry.
+ */
+function ColumnMultiControl(props: ControlProps): ReactElement {
+ const datasetId = useBoundDatasetId(props);
+ const { metadata, loading, error } = useDatasetMetadata(datasetId);
+ const allowedTypes = (props.schema as Record<string, unknown>)[
+ 'x-column-types'
+ ] as string[] | undefined;
+ const values = Array.isArray(props.data) ? (props.data as unknown[]) : [];
+ const hasNonStringEntry = values.some(value => typeof value !== 'string');
+
+ if (shouldFallBackToCode(datasetId, error) || hasNonStringEntry) {
+ return <CodeControl {...props} />;
+ }
+
+ return (
+ <ReferenceMultiList
+ label={props.label}
+ description={props.description}
+ values={values as string[]}
+ options={columnOptions(metadata, allowedTypes)}
+ loading={loading}
+ disabled={!props.enabled}
+ onChange={next => props.handleChange(props.path, next)}
+ />
+ );
+}
+
+/**
+ * Metric options for a `metric-multi` control: the dataset's saved metrics,
+ * shown with the same Sigma icon Explore's metric picker uses.
+ */
+export function metricOptions(
+ metadata: DatasetMetadata | null,
+): ReferenceOption[] {
+ return (metadata?.metrics ?? []).map(metric => ({
+ value: metric.name,
+ label: (
+ <Flex align="center" gap="small">
+ <ColumnTypeLabel type="metric" />
+ {metric.verboseName}
+ </Flex>
+ ),
+ }));
+}
+
+/**
+ * True for a metric entry this control has no row to draw at all: neither a
+ * plain saved-metric-name string nor a structurally valid ad-hoc metric
+ * object — e.g. malformed data hand-authored through the JSON tab. Only
+ * this case still drops the *whole* field to the raw JSON editor; a
+ * well-formed mix of saved and ad-hoc entries renders as a mixed list
+ * instead (see `MetricEntryList`).
+ */
+export function isUnrepresentableMetric(value: unknown): boolean {
+ return typeof value !== 'string' && !isDictionaryForAdhocMetric(value);
+}
+
+/** Whether the bound dataset's own settings forbid ad-hoc metrics entirely
+ * (a dataset-level admin setting, not a per-field one) — read the same raw
+ * `extra` JSON the legacy metric editor reads it from. */
+export function disallowsAdhocMetrics(
+ metadata: DatasetMetadata | null,
+): boolean {
+ if (!metadata?.extra) return false;
+ try {
+ return Boolean(
+ (JSON.parse(metadata.extra) as { disallow_adhoc_metrics?: boolean })
+ .disallow_adhoc_metrics,
+ );
+ } catch {
+ return false;
+ }
+}
+
+type MetricEntry = string | CoreAdhocMetric;
+
+/** A metric entry's own display label: a saved metric's verbose name (or
+ * its raw name if the dataset's metric list hasn't loaded/matched yet), or
+ * an ad-hoc metric's own label — computed the same way the legacy editor
+ * derives one (`(AVG)(price)`, etc.) when the author hasn't set a custom one. */
+function metricEntryLabel(
+ entry: MetricEntry,
+ metadata: DatasetMetadata | null,
+): ReactNode {
+ if (typeof entry === 'string') {
+ const known = metadata?.metrics.find(metric => metric.name === entry);
+ return known?.verboseName ?? entry;
+ }
+ return fromCoreAdhocMetric(entry).label;
+}
+
+/** Sentinel option value picked from the "Add field" select to start a new
+ * ad-hoc metric, distinct from any real saved-metric name. */
+const ADD_CUSTOM_METRIC = '__custom_metric__';
+
+/**
+ * An ordered list of metric references, each entry rendered by its own
+ * kind: a saved metric as a plain row (unchanged from `ReferenceMultiList`),
+ * an ad-hoc metric (SIMPLE or SQL) as a row whose label opens
+ * `AdhocMetricEditor` for just that entry. "Add field" offers both a saved
+ * metric to pick and, unless the dataset disallows it, a blank ad-hoc draft.
+ */
+function MetricEntryList({
+ label,
+ description,
+ values,
+ metadata,
+ columns,
+ datasourceId,
+ datasourceType,
+ disallowAdhoc,
+ disabled,
+ onChange,
+}: {
+ label: string;
+ description: string | undefined;
+ values: MetricEntry[];
+ metadata: DatasetMetadata | null;
+ columns: DatasetColumnMeta[];
+ datasourceId: number | undefined;
+ datasourceType: string | undefined;
+ disallowAdhoc: boolean;
+ disabled: boolean;
+ onChange: (next: MetricEntry[]) => void;
+}): ReactElement {
+ const [editingIndex, setEditingIndex] = useState<number | null>(null);
+ const [addingNew, setAddingNew] = useState(false);
+
+ const pickedSavedNames = values.filter(
+ (value): value is string => typeof value === 'string',
+ );
+ const availableSaved = metricOptions(metadata).filter(
+ option => !pickedSavedNames.includes(option.value),
+ );
+ const addOptions = [
+ ...availableSaved,
+ ...(disallowAdhoc
+ ? []
+ : [{ value: ADD_CUSTOM_METRIC, label: t('Custom metric…') }]),
+ ];
+
+ const move = (from: number, to: number) => {
+ const next = [...values];
+ const [moved] = next.splice(from, 1);
+ next.splice(to, 0, moved);
+ onChange(next);
+ };
+
+ return (
+ <Form.Item label={label} tooltip={description}>
+ <Flex vertical gap="small">
+ {values.map((value, index) => {
+ const isAdhoc = typeof value !== 'string';
+ const canEdit = isAdhoc && !disallowAdhoc;
+ const key =
+ typeof value === 'string'
+ ? value
+ : (value.optionName ?? `adhoc-${index}`);
+ // Event handlers only attached at all when `canEdit` — a static
+ // div with an onClick/onKeyDown regardless of role is what the
+ // a11y linter (rightly) objects to, not just a style choice.
+ const interactiveProps = canEdit
+ ? {
+ role: 'button' as const,
+ tabIndex: 0,
+ onClick: () => setEditingIndex(index),
+ onKeyDown: (event: KeyboardEvent<HTMLDivElement>) => {
+ if (event.key === 'Enter' || event.key === ' ') {
+ event.preventDefault();
+ setEditingIndex(index);
+ }
+ },
+ }
+ : {};
+ const rowLabel = (
+ <div
+ style={{
+ flex: 1,
+ minWidth: 0,
+ cursor: canEdit ? 'pointer' : 'default',
+ }}
+ {...interactiveProps}
+ >
+ <Flex align="center" gap="small">
+ <ColumnTypeLabel type="metric" />
+ {metricEntryLabel(value, metadata)}
+ </Flex>
+ </div>
+ );
+ return (
+ <Flex key={key} align="center" gap="small">
+ <Icons.HolderOutlined iconSize="s" />
+ {isAdhoc ? (
+ <AdhocMetricEditor
+ value={value}
+ columns={columns}
+ datasourceId={datasourceId}
+ datasourceType={datasourceType}
+ open={editingIndex === index}
+ onOpenChange={open => {
+ // `Popover`'s own `trigger="click"` opens on any click to
+ // its children regardless of what the row's own onClick
+ // does — `canEdit` has to gate here too, or a disallowed
+ // dataset's rows would still open on click.
+ if (canEdit) setEditingIndex(open ? index : null);
+ }}
+ onSave={next => {
+ const updated = [...values];
+ updated[index] = next;
+ onChange(updated);
+ }}
+ >
+ {rowLabel}
+ </AdhocMetricEditor>
+ ) : (
+ rowLabel
+ )}
+ <Button
+ buttonSize="xsmall"
+ buttonStyle="link"
+ aria-label={t('Move metric %s up', index + 1)}
+ disabled={index === 0}
+ icon={<Icons.UpOutlined iconSize="s" />}
+ onClick={() => move(index, index - 1)}
+ />
+ <Button
+ buttonSize="xsmall"
+ buttonStyle="link"
+ aria-label={t('Move metric %s down', index + 1)}
+ disabled={index === values.length - 1}
+ icon={<Icons.DownOutlined iconSize="s" />}
+ onClick={() => move(index, index + 1)}
+ />
+ <Button
+ buttonSize="xsmall"
+ buttonStyle="link"
+ aria-label={t('Remove metric %s', index + 1)}
+ icon={<Icons.CloseOutlined iconSize="s" />}
+ onClick={() => onChange(values.filter((_, i) => i !== index))}
+ />
+ </Flex>
+ );
+ })}
+ {addOptions.length > 0 && (
+ <Select
+ key={`${availableSaved.length}-${values.length}`}
Review Comment:
Picking `Custom metric…` changes neither `availableSaved.length` nor `values.length`, so the remount that clears rc-select's echoed label (per your note at line 677) never fires and the box keeps reading "Custom metric…" after Cancel.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org