rusackas commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3229989337
##########
superset-frontend/src/explore/components/controls/TextAreaControl.tsx:
##########
@@ -74,207 +67,259 @@ interface TextAreaControlProps {
tooltipOptions?: Record<string, unknown>;
hotkeys?: HotkeyConfig[];
debounceDelay?: number | null;
- theme?: ThemeType;
'aria-required'?: boolean;
value?: string;
[key: string]: unknown;
}
-const defaultProps = {
- onChange: () => {},
- height: 250,
- minLines: 3,
- maxLines: 10,
- offerEditInModal: true,
- readOnly: false,
- resize: null,
- textAreaStyles: {},
- tooltipOptions: {},
- hotkeys: [],
- debounceDelay: null,
-};
-
-class TextAreaControl extends Component<TextAreaControlProps> {
- static defaultProps = defaultProps;
+function TextAreaControl({
+ name,
+ onChange = () => {},
+ initialValue,
+ height = 250,
+ minLines = 3,
+ maxLines = 10,
+ offerEditInModal = true,
+ language,
+ aboveEditorSection,
+ readOnly = false,
+ resize = null,
+ textAreaStyles = {},
+ tooltipOptions = {},
+ hotkeys = [],
+ debounceDelay = null,
+ 'aria-required': ariaRequired,
+ value,
+ ...restProps
+}: TextAreaControlProps) {
+ const theme = useTheme();
- debouncedOnChange:
- | ReturnType<typeof debounce<(value: string) => void>>
- | undefined;
+ const debouncedOnChangeRef = useRef<ReturnType<
+ typeof debounce<(value: string) => void>
+ > | null>(null);
- constructor(props: TextAreaControlProps) {
- super(props);
- if (props.debounceDelay && props.onChange) {
- this.debouncedOnChange = debounce(props.onChange, props.debounceDelay);
- }
- }
-
- componentDidUpdate(prevProps: TextAreaControlProps) {
- if (
- this.props.onChange !== prevProps.onChange &&
- this.props.debounceDelay &&
- this.props.onChange
- ) {
- if (this.debouncedOnChange) {
- this.debouncedOnChange.cancel();
+ // Create or update debounced onChange when dependencies change
+ useEffect(() => {
+ if (debounceDelay && onChange) {
+ if (debouncedOnChangeRef.current) {
+ debouncedOnChangeRef.current.cancel();
}
- this.debouncedOnChange = debounce(
- this.props.onChange,
- this.props.debounceDelay,
- );
- }
- }
-
- handleChange(value: string | { target: { value: string } }) {
- const finalValue = typeof value === 'object' ? value.target.value : value;
- if (this.debouncedOnChange) {
- this.debouncedOnChange(finalValue);
+ debouncedOnChangeRef.current = debounce(onChange, debounceDelay);
} else {
- this.props.onChange?.(finalValue);
- }
- }
-
- componentWillUnmount() {
- if (this.debouncedOnChange) {
- this.debouncedOnChange.flush();
+ if (debouncedOnChangeRef.current) {
+ debouncedOnChangeRef.current.cancel();
+ }
+ debouncedOnChangeRef.current = null;
}
- }
+ }, [onChange, debounceDelay]);
- renderEditor(inModal = false) {
- // Exclude props that shouldn't be passed to TextAreaEditor:
- // - theme: TextAreaEditor expects theme as a string, not the theme object
from withTheme HOC
- // - height: ReactAce expects string, we pass number (height is controlled
via minLines/maxLines)
- // - other control-specific props and explicitly-set props to avoid
duplicate/conflicting assignments
- const {
- theme,
- height,
- offerEditInModal,
- aboveEditorSection,
- resize,
- textAreaStyles,
- tooltipOptions,
- hotkeys,
- debounceDelay,
- language,
- initialValue,
- readOnly,
- name,
- onChange,
- value,
- minLines: minLinesProp,
- maxLines: maxLinesProp,
- ...editorProps
- } = this.props;
- const minLines = inModal ? 40 : minLinesProp || 12;
- if (language) {
- const style: React.CSSProperties = {
- border: theme?.colorBorder
- ? `1px solid ${theme.colorBorder}`
- : undefined,
- minHeight: `${minLines}em`,
- width: 'auto',
- ...textAreaStyles,
- };
- if (resize) {
- style.resize = resize;
- style.overflow = 'auto';
+ // Cleanup on unmount — flush pending debounced onChange so last edit isn't
lost
+ useEffect(
+ () => () => {
+ if (debouncedOnChangeRef.current) {
+ debouncedOnChangeRef.current.flush();
}
- if (readOnly) {
- style.backgroundColor = theme?.colorBgMask;
+ },
+ [],
+ );
+
+ const handleChange = useCallback(
+ (val: string | { target: { value: string } }) => {
+ const finalValue = typeof val === 'object' ? val.target.value : val;
+ if (debouncedOnChangeRef.current) {
+ debouncedOnChangeRef.current(finalValue);
+ } else {
+ onChange?.(finalValue);
}
- const onEditorLoad = (editor: {
- commands: {
- addCommand: (cmd: {
- name: string;
- bindKey: { win: string; mac: string };
- exec: () => void;
- }) => void;
- };
- }) => {
- hotkeys?.forEach(keyConfig => {
- editor.commands.addCommand({
- name: keyConfig.name,
- bindKey: { win: keyConfig.key, mac: keyConfig.key },
- exec: keyConfig.func,
- });
- });
+ },
+ [onChange],
+ );
+
+ const onEditorLoad = useCallback(
+ (editor: {
+ commands: {
+ addCommand: (cmd: {
+ name: string;
+ bindKey: { win: string; mac: string };
+ exec: () => void;
+ }) => void;
};
- const codeEditor = (
+ }) => {
+ hotkeys?.forEach(keyConfig => {
+ editor.commands.addCommand({
+ name: keyConfig.name,
+ bindKey: { win: keyConfig.key, mac: keyConfig.key },
+ exec: keyConfig.func,
+ });
+ });
+ },
+ [hotkeys],
+ );
+
+ const renderEditor = useCallback(
+ (inModal = false) => {
+ const effectiveMinLines = inModal ? 40 : minLines || 12;
+
+ if (language) {
+ const style: React.CSSProperties = {
+ border: theme?.colorBorder
+ ? `1px solid ${theme.colorBorder}`
+ : undefined,
+ minHeight: `${effectiveMinLines}em`,
+ width: 'auto',
+ ...textAreaStyles,
+ };
+
+ if (resize) {
+ style.resize = resize;
+ style.overflow = 'auto';
+ }
+
+ if (readOnly) {
+ style.backgroundColor = theme?.colorBgMask;
+ }
+
+ const codeEditor = (
+ <div>
+ <TextAreaEditor
+ mode={language}
+ style={style}
+ minLines={effectiveMinLines}
+ maxLines={inModal ? 1000 : maxLines}
+ editorProps={{ $blockScrolling: true }}
+ onLoad={onEditorLoad}
+ defaultValue={initialValue ?? value}
+ readOnly={readOnly}
+ key={name}
+ {...restProps}
+ onChange={handleChange}
+ />
+ </div>
+ );
+
+ if (tooltipOptions && Object.keys(tooltipOptions).length > 0) {
+ return <Tooltip {...tooltipOptions}>{codeEditor}</Tooltip>;
+ }
+ return codeEditor;
+ }
+
+ const textArea = (
<div>
- <TextAreaEditor
- mode={language}
- style={style}
- minLines={minLines}
- maxLines={inModal ? 1000 : maxLinesProp}
- editorProps={{ $blockScrolling: true }}
- onLoad={onEditorLoad}
+ <Input.TextArea
+ placeholder={t('textarea')}
+ onChange={handleChange}
defaultValue={initialValue ?? value}
- readOnly={readOnly}
- key={name}
- {...editorProps}
- onChange={this.handleChange.bind(this)}
+ disabled={readOnly}
+ style={{ height }}
+ aria-required={ariaRequired}
/>
</div>
);
- if (tooltipOptions) {
- return <Tooltip {...tooltipOptions}>{codeEditor}</Tooltip>;
+ if (tooltipOptions && Object.keys(tooltipOptions).length > 0) {
+ return <Tooltip {...tooltipOptions}>{textArea}</Tooltip>;
}
- return codeEditor;
- }
+ return textArea;
+ },
+ [
+ minLines,
+ maxLines,
+ language,
+ theme,
+ textAreaStyles,
+ resize,
+ readOnly,
+ onEditorLoad,
+ initialValue,
+ value,
+ name,
+ restProps,
+ handleChange,
+ tooltipOptions,
+ height,
+ ariaRequired,
+ ],
+ );
- const textArea = (
- <div>
- <Input.TextArea
- placeholder={t('textarea')}
- onChange={this.handleChange.bind(this)}
- defaultValue={this.props.initialValue}
- disabled={this.props.readOnly}
- style={{ height: this.props.height }}
- aria-required={this.props['aria-required']}
- />
- </div>
- );
- if (this.props.tooltipOptions) {
- return <Tooltip {...this.props.tooltipOptions}>{textArea}</Tooltip>;
- }
- return textArea;
- }
+ // Extract only ControlHeader-compatible props from restProps
+ const {
+ label,
+ description,
+ validationErrors,
+ renderTrigger,
+ rightNode,
+ leftNode,
+ onClick,
+ hovered,
+ tooltipOnClick,
+ warning,
+ danger,
+ } = restProps as Record<string, unknown>;
+
+ const controlHeader = useMemo(
+ () => (
+ <ControlHeader
+ name={name}
+ label={label as React.ReactNode}
+ description={description as React.ReactNode}
+ validationErrors={validationErrors as string[] | undefined}
+ renderTrigger={renderTrigger as boolean | undefined}
+ rightNode={rightNode as React.ReactNode}
+ leftNode={leftNode as React.ReactNode}
+ onClick={onClick as (() => void) | undefined}
+ hovered={hovered as boolean | undefined}
+ tooltipOnClick={tooltipOnClick as (() => void) | undefined}
+ warning={warning as string | undefined}
+ danger={danger as string | undefined}
+ />
+ ),
+ [
+ name,
+ label,
+ description,
+ validationErrors,
+ renderTrigger,
+ rightNode,
+ leftNode,
+ onClick,
+ hovered,
+ tooltipOnClick,
+ warning,
+ danger,
+ ],
+ );
- renderModalBody() {
- return (
+ const modalBody = useMemo(
+ () => (
<>
- <div>{this.props.aboveEditorSection}</div>
- {this.renderEditor(true)}
+ <div>{aboveEditorSection}</div>
+ {renderEditor(true)}
</>
- );
- }
+ ),
+ [aboveEditorSection, renderEditor],
+ );
- render() {
- const controlHeader = <ControlHeader {...this.props} />;
- return (
- <div>
- {controlHeader}
- {this.renderEditor()}
- {this.props.offerEditInModal && (
- <ModalTrigger
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- modalTitle={controlHeader as any}
- triggerNode={
- <Button
- buttonSize="small"
- style={{ marginTop: this.props.theme?.sizeUnit ?? 4 }}
- >
- {t('Edit %s in modal', this.props.language)}
- </Button>
- }
- modalBody={this.renderModalBody()}
- responsive
- />
- )}
- </div>
- );
- }
+ return (
+ <div>
+ {controlHeader}
+ {renderEditor()}
+ {offerEditInModal && (
+ <ModalTrigger
+ modalTitle={String(label || '')}
Review Comment:
Done in 62808305f8 — widened `ModalTrigger.modalTitle` to `ReactNode`,
dropped the 12-prop destructure for `<ControlHeader name={name} {...restProps}
/>` (same shape as `ViewportControl` uses elsewhere in this PR), and pass
`controlHeader` as modalTitle to restore the description tooltip / validation
badges / warning icons that master had via `as any`.
##########
superset-frontend/src/explore/components/controls/SelectControl.tsx:
##########
@@ -300,46 +316,69 @@ export default class SelectControl extends PureComponent<
disabled,
filterOption:
filterOption && typeof filterOption === 'function'
- ? this.handleFilterOptions
+ ? handleFilterOptions
: true,
header: showHeader && <ControlHeader {...headerProps} />,
loading: isLoading,
- mode: this.props.mode || (isMulti || multi ? 'multiple' : 'single'),
+ mode: mode || (isMulti || multi ? 'multiple' : 'single'),
name: `select-${name}`,
- onChange: this.onChange,
+ onChange: handleChange,
onFocus,
onSelect,
onDeselect,
- options: this.state.options,
+ options,
placeholder,
- sortComparator: getSortComparator(
- this.props.choices,
- this.props.options,
- this.props.valueKey,
- this.props.sortComparator,
- ),
+ sortComparator: computedSortComparator,
value: getValue(),
tokenSeparators,
notFoundContent,
- };
+ }),
+ [
+ freeForm,
+ autoFocus,
+ ariaLabel,
+ label,
+ clearable,
+ disabled,
+ filterOption,
+ handleFilterOptions,
+ showHeader,
+ headerProps,
+ isLoading,
+ mode,
+ isMulti,
+ multi,
+ name,
+ handleChange,
+ onFocus,
+ onSelect,
+ onDeselect,
+ options,
+ placeholder,
+ computedSortComparator,
+ getValue,
+ tokenSeparators,
+ notFoundContent,
+ ],
+ );
- return (
- <div
- css={theme => css`
- .type-label {
- margin-right: ${theme.sizeUnit * 2}px;
- }
- .Select__multi-value__label > span,
- .Select__option > span,
- .Select__single-value > span {
- display: flex;
- align-items: center;
- }
- `}
- >
- {/* eslint-disable-next-line @typescript-eslint/no-explicit-any */}
- <Select {...(selectProps as any)} />
- </div>
- );
- }
+ return (
+ <div
+ css={theme => css`
+ .type-label {
+ margin-right: ${theme.sizeUnit * 2}px;
+ }
+ .Select__multi-value__label > span,
+ .Select__option > span,
+ .Select__single-value > span {
+ display: flex;
+ align-items: center;
+ }
+ `}
+ >
+ <Select {...(selectProps as Parameters<typeof Select>[0])} />
+ </div>
+ );
}
+
+export default SelectControl;
Review Comment:
Done in 62d70a113b — wrapped `SelectControl`, `AnnotationLayerControl`
(inside the `connect` HOC), `AdhocFilterPopoverTrigger`,
`FixedOrMetricControl`, and `MetricsControl` with `memo()` to preserve the
`PureComponent` skip behavior. `AdhocMetricEditPopover` and
`AdhocMetricPopoverTrigger` already had `memo` in place.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -882,602 +833,694 @@ const mapStateToProps = (state: RootState) => ({
const connector = connect(mapStateToProps, mapDispatchToProps);
type PropsFromRedux = ConnectedProps<typeof connector>;
-type DatasourceEditorProps = DatasourceEditorOwnProps &
- PropsFromRedux & {
- theme?: SupersetTheme;
- };
-
-class DatasourceEditor extends PureComponent<
- DatasourceEditorProps,
- DatasourceEditorState
-> {
- private isComponentMounted: boolean;
+type DatasourceEditorProps = DatasourceEditorOwnProps & PropsFromRedux;
+
+function DatasourceEditor({
+ datasource: propsDatasource,
+ onChange = () => {},
+ addSuccessToast,
+ addDangerToast,
+ setIsEditing = () => {},
+ database,
+ runQuery,
+ resetQuery,
+ formatQuery: formatQueryAction,
+}: DatasourceEditorProps) {
+ const theme = useTheme();
+ const isComponentMounted = useRef(false);
+ const isInitialMount = useRef(true);
+ const prevPropsDatasourceRef = useRef(propsDatasource);
+ const isSyncingColumnsFromProps = useRef(false);
+ const abortControllers = useRef<AbortControllers>({
+ formatQuery: null,
+ formatSql: null,
+ syncMetadata: null,
+ fetchUsageData: null,
+ });
+
+ // Initialize datasource state with transformed owners and metrics
+ const [datasource, setDatasource] = useState<DatasourceObject>(() => ({
+ ...propsDatasource,
+ owners: propsDatasource.owners.map(owner => ({
+ value: owner.value || owner.id,
+ label: owner.label || `${owner.first_name} ${owner.last_name}`,
+ })),
+ metrics: propsDatasource.metrics?.map(metric => {
+ const {
+ certified_by: certifiedByMetric,
+ certification_details: certificationDetails,
+ } = metric;
+ const {
+ certification: {
+ details = undefined,
+ certified_by: certifiedBy = undefined,
+ } = {},
+ warning_markdown: warningMarkdown,
+ } = JSON.parse(metric.extra || '{}') || {};
+ return {
+ ...metric,
+ certification_details: certificationDetails || details,
+ warning_markdown: warningMarkdown || '',
+ certified_by: certifiedBy || certifiedByMetric,
+ };
+ }),
+ }));
- private abortControllers: AbortControllers;
+ const [errors, setErrors] = useState<string[]>([]);
+ const [isSqla] = useState(
+ propsDatasource.datasource_type === 'table' ||
+ propsDatasource.type === 'table',
+ );
+ const [isEditMode, setIsEditMode] = useState(false);
+ const [databaseColumns, setDatabaseColumns] = useState<Column[]>(
+ propsDatasource.columns.filter(col => !col.expression),
+ );
+ const [calculatedColumns, setCalculatedColumns] = useState<Column[]>(
+ propsDatasource.columns.filter(col => !!col.expression),
+ );
+ const [folders, setFolders] = useState<DatasourceFolder[]>(
+ propsDatasource.folders || [],
+ );
+ const [folderCount, setFolderCount] = useState(() => {
+ const savedFolders = propsDatasource.folders || [];
+ const savedCount = countAllFolders(savedFolders);
+ const hasDefaultsSaved = savedFolders.some(f => isDefaultFolder(f.uuid));
+ return savedCount + (hasDefaultsSaved ? 0 : DEFAULT_FOLDERS_COUNT);
+ });
+ const [metadataLoading, setMetadataLoading] = useState(false);
+ const [activeTabKey, setActiveTabKey] = useState(TABS_KEYS.SOURCE);
+ const [datasourceType, setDatasourceType] = useState(
+ propsDatasource.sql
+ ? DATASOURCE_TYPES.virtual.key
+ : DATASOURCE_TYPES.physical.key,
+ );
+ const [usageCharts, setUsageCharts] = useState<ChartUsageData[]>([]);
+ const [usageChartsCount, setUsageChartsCount] = useState(0);
+ const [metricSearchTerm, setMetricSearchTerm] = useState('');
+ const [columnSearchTerm, setColumnSearchTerm] = useState('');
+ const [calculatedColumnSearchTerm, setCalculatedColumnSearchTerm] =
+ useState('');
+
+ const findDuplicates = useCallback(
+ <T,>(arr: T[], accessor: (obj: T) => string): string[] => {
+ const seen: Record<string, null> = {};
+ const dups: string[] = [];
+ arr.forEach((obj: T) => {
+ const item = accessor(obj);
+ if (item in seen) {
+ dups.push(item);
+ } else {
+ seen[item] = null;
+ }
+ });
+ return dups;
+ },
+ [],
+ );
- static defaultProps = {
- onChange: () => {},
- setIsEditing: () => {},
- };
+ const validate = useCallback(
+ (callback: (validationErrors: string[]) => void) => {
+ let validationErrors: string[] = [];
+ let dups: string[];
- constructor(props: DatasourceEditorProps) {
- super(props);
- this.state = {
- datasource: {
- ...props.datasource,
- owners: props.datasource.owners.map(owner => {
- const ownerName =
- owner.label || `${owner.first_name} ${owner.last_name}`;
- return {
- value: owner.value || owner.id,
- label: OwnerSelectLabel({
- name: typeof ownerName === 'string' ? ownerName : '',
- email: owner.email,
- }),
- [OWNER_TEXT_LABEL_PROP]:
- typeof ownerName === 'string' ? ownerName : '',
- [OWNER_EMAIL_PROP]: owner.email ?? '',
- };
- }),
- metrics: props.datasource.metrics?.map(metric => {
- const {
- certified_by: certifiedByMetric,
- certification_details: certificationDetails,
- } = metric;
- const {
- certification: {
- details = undefined,
- certified_by: certifiedBy = undefined,
- } = {},
- warning_markdown: warningMarkdown,
- } = JSON.parse(metric.extra || '{}') || {};
- return {
- ...metric,
- certification_details: certificationDetails || details,
- warning_markdown: warningMarkdown || '',
- certified_by: certifiedBy || certifiedByMetric,
- };
- }),
- },
- errors: [],
- isSqla:
- props.datasource.datasource_type === 'table' ||
- props.datasource.type === 'table',
- isEditMode: false,
- databaseColumns: props.datasource.columns.filter(col => !col.expression),
- calculatedColumns: props.datasource.columns.filter(
- col => !!col.expression,
- ),
- folders: props.datasource.folders || [],
- folderCount: (() => {
- const savedFolders = props.datasource.folders || [];
- const savedCount = countAllFolders(savedFolders);
- const hasDefaultsSaved = savedFolders.some(f =>
- isDefaultFolder(f.uuid),
- );
- return savedCount + (hasDefaultsSaved ? 0 : DEFAULT_FOLDERS_COUNT);
- })(),
- metadataLoading: false,
- activeTabKey: TABS_KEYS.SOURCE,
- datasourceType: props.datasource.sql
- ? DATASOURCE_TYPES.virtual.key
- : DATASOURCE_TYPES.physical.key,
- usageCharts: [],
- usageChartsCount: 0,
- metricSearchTerm: '',
- columnSearchTerm: '',
- calculatedColumnSearchTerm: '',
- };
+ // Looking for duplicate column_name
+ dups = findDuplicates(datasource.columns, obj => obj.column_name);
+ validationErrors = validationErrors.concat(
+ dups.map(name => t('Column name [%s] is duplicated', name)),
+ );
- this.isComponentMounted = false;
- this.abortControllers = {
- formatQuery: null,
- formatSql: null,
- syncMetadata: null,
- fetchUsageData: null,
- };
+ // Looking for duplicate metric_name
+ dups = findDuplicates(datasource.metrics ?? [], obj => obj.metric_name);
+ validationErrors = validationErrors.concat(
+ dups.map(name => t('Metric name [%s] is duplicated', name)),
+ );
- this.onChange = this.onChange.bind(this);
- this.onChangeEditMode = this.onChangeEditMode.bind(this);
- this.onDatasourcePropChange = this.onDatasourcePropChange.bind(this);
- this.onDatasourceChange = this.onDatasourceChange.bind(this);
- this.tableChangeAndSyncMetadata =
- this.tableChangeAndSyncMetadata.bind(this);
- this.syncMetadata = this.syncMetadata.bind(this);
- this.setColumns = this.setColumns.bind(this);
- this.validateAndChange = this.validateAndChange.bind(this);
- this.handleTabSelect = this.handleTabSelect.bind(this);
- this.formatSql = this.formatSql.bind(this);
- this.fetchUsageData = this.fetchUsageData.bind(this);
- this.handleFoldersChange = this.handleFoldersChange.bind(this);
- }
+ // Making sure calculatedColumns have an expression defined
+ const noFilterCalcCols = calculatedColumns.filter(
+ col => !col.expression && !col.json,
+ );
+ validationErrors = validationErrors.concat(
+ noFilterCalcCols.map(col =>
+ t('Calculated column [%s] requires an expression', col.column_name),
+ ),
+ );
- onChange() {
- // Emptying SQL if "Physical" radio button is selected
- // Currently the logic to know whether the source is
- // physical or virtual is based on whether SQL is empty or not.
- const { datasourceType, datasource } = this.state;
- const sql =
- datasourceType === DATASOURCE_TYPES.physical.key ? '' : datasource.sql;
-
- const columns = [
- ...this.state.databaseColumns,
- ...this.state.calculatedColumns,
- ];
-
- // Remove deleted column/metric references from folders
- const validUuids = new Set<string>();
- for (const col of columns) {
- if (col.uuid) validUuids.add(col.uuid);
- }
- for (const metric of datasource.metrics ?? []) {
- if (metric.uuid) validUuids.add(metric.uuid);
- }
- const folders = filterFoldersByValidUuids(this.state.folders, validUuids);
+ // validate currency code (skip 'AUTO' - it's a placeholder for
auto-detection)
+ try {
+ datasource.metrics?.forEach(
+ metric =>
+ metric.currency?.symbol &&
+ metric.currency.symbol !== 'AUTO' &&
+ new Intl.NumberFormat('en-US', {
+ style: 'currency',
+ currency: metric.currency.symbol,
+ }),
+ );
+ } catch {
+ validationErrors = validationErrors.concat([
+ t('Invalid currency code in saved metrics'),
+ ]);
+ }
- const newDatasource = {
- ...this.state.datasource,
- sql,
- columns,
- folders,
- };
+ // Validate folders
+ if (folders?.length > 0) {
+ const folderValidation = validateFolders(folders);
+ validationErrors = validationErrors.concat(folderValidation.errors);
+ }
- this.props.onChange?.(newDatasource, this.state.errors);
- }
+ setErrors(validationErrors);
+ callback(validationErrors);
+ },
+ [datasource, calculatedColumns, folders, findDuplicates],
+ );
- onChangeEditMode() {
- this.props.setIsEditing?.(!this.state.isEditMode);
- this.setState(prevState => ({ isEditMode: !prevState.isEditMode }));
- }
+ const onChangeInternal = useCallback(
+ (validationErrors: string[] = errors) => {
+ // Emptying SQL if "Physical" radio button is selected
+ const sql =
+ datasourceType === DATASOURCE_TYPES.physical.key ? '' : datasource.sql;
- onDatasourceChange(
- datasource: DatasourceObject,
- callback: () => void = this.validateAndChange,
- ) {
- this.setState({ datasource }, callback);
- }
+ const columns = [...databaseColumns, ...calculatedColumns];
- onDatasourcePropChange(attr: string, value: unknown) {
- if (value === undefined) return; // if value is undefined do not update
state
- const datasource = { ...this.state.datasource, [attr]: value };
- this.setState(
- prevState => ({
- datasource: { ...prevState.datasource, [attr]: value },
- }),
- () =>
- attr === 'table_name'
- ? this.onDatasourceChange(datasource,
this.tableChangeAndSyncMetadata)
- : this.onDatasourceChange(datasource, this.validateAndChange),
- );
- }
+ // Remove deleted column/metric references from folders
+ const validUuids = new Set<string>();
+ for (const col of columns) {
+ if (col.uuid) validUuids.add(col.uuid);
+ }
+ for (const metric of datasource.metrics ?? []) {
+ if (metric.uuid) validUuids.add(metric.uuid);
+ }
+ const filteredFolders = filterFoldersByValidUuids(folders, validUuids);
- onDatasourceTypeChange(datasourceType: string) {
- // Call onChange after setting datasourceType to ensure
- // SQL is cleared when switching to a physical dataset
- this.setState({ datasourceType }, this.onChange);
- }
+ const newDatasource = {
+ ...datasource,
+ sql,
+ columns,
+ folders: filteredFolders,
+ };
- handleFoldersChange(folders: DatasourceFolder[]) {
- const folderCount = countAllFolders(folders);
- this.setState({ folders, folderCount }, () => {
- this.onDatasourceChange({
- ...this.state.datasource,
- folders,
- });
- });
- }
+ onChange(newDatasource, validationErrors);
+ },
+ [
+ datasource,
+ datasourceType,
+ databaseColumns,
+ calculatedColumns,
+ folders,
+ errors,
+ onChange,
+ ],
+ );
- setColumns(
- obj: { databaseColumns?: Column[] } | { calculatedColumns?: Column[] },
- ) {
- // update calculatedColumns or databaseColumns
- this.setState(
- obj as Pick<
- DatasourceEditorState,
- 'databaseColumns' | 'calculatedColumns'
- >,
- this.validateAndChange,
- );
- }
+ const validateAndChange = useCallback(() => {
+ validate(onChangeInternal);
+ }, [validate, onChangeInternal]);
- validateAndChange() {
- this.validate(this.onChange);
- }
+ const onDatasourceChange = useCallback((newDatasource: DatasourceObject) => {
+ setDatasource(newDatasource);
+ }, []);
- async onQueryRun() {
- const databaseId = this.state.datasource.database?.id;
- const { sql } = this.state.datasource;
- if (!databaseId || !sql) {
- return;
- }
- this.props.runQuery({
- client_id: this.props.database?.clientId,
- database_id: databaseId,
- runAsync: false,
- catalog: this.state.datasource.catalog,
- schema: this.state.datasource.schema,
- sql,
- tmp_table_name: '',
- select_as_cta: false,
- ctas_method: 'TABLE',
- queryLimit: 25,
- expand_data: true,
+ const onDatasourcePropChange = useCallback((attr: string, value: unknown) =>
{
+ if (value === undefined) return;
+ setDatasource(prev => {
+ const newDatasource = { ...prev, [attr]: value };
+ return newDatasource;
});
- }
+ }, []);
- /**
- * Formats SQL query using the formatQuery action.
- * Aborts any pending format requests before starting a new one.
- */
- async onQueryFormat() {
- const { datasource } = this.state;
- if (!datasource.sql || !this.state.isEditMode) {
+ // Effect to trigger validation after datasource changes (skip initial mount)
+ useEffect(() => {
+ if (isInitialMount.current) {
return;
}
-
- // Abort previous formatQuery if still pending
- if (this.abortControllers.formatQuery) {
- this.abortControllers.formatQuery.abort();
+ if (isComponentMounted.current) {
+ validateAndChange();
}
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [datasource]);
- this.abortControllers.formatQuery = new AbortController();
- const { signal } = this.abortControllers.formatQuery;
+ const onChangeEditMode = useCallback(() => {
+ setIsEditing(!isEditMode);
+ setIsEditMode(prev => !prev);
+ }, [isEditMode, setIsEditing]);
- try {
- const response = await this.props.formatQuery(datasource.sql, { signal
});
+ const onDatasourceTypeChange = useCallback((newDatasourceType: string) => {
+ setDatasourceType(newDatasourceType);
+ }, []);
- this.onDatasourcePropChange('sql', response.json.result);
- this.props.addSuccessToast(t('SQL was formatted'));
- } catch (error) {
- if (error.name === 'AbortError') return;
+ // Effect to call onChange after datasourceType changes (skip initial mount)
+ useEffect(() => {
+ if (isInitialMount.current) {
+ return;
+ }
+ if (isComponentMounted.current) {
+ onChangeInternal();
+ }
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [datasourceType]);
+
+ const handleFoldersChange = useCallback((newFolders: DatasourceFolder[]) => {
+ const userMadeFolders = newFolders.filter(
+ f =>
+ f.uuid !== DEFAULT_METRICS_FOLDER_UUID &&
+ f.uuid !== DEFAULT_COLUMNS_FOLDER_UUID &&
+ (f.children?.length ?? 0) > 0,
+ );
+ setFolders(userMadeFolders);
+ setFolderCount(countAllFolders(userMadeFolders));
+ setDatasource(prev => ({ ...prev, folders: userMadeFolders }));
+ }, []);
- const { error: clientError, statusText } =
- await getClientErrorObject(error);
+ const setColumns = useCallback(
+ (
+ obj: { databaseColumns?: Column[] } | { calculatedColumns?: Column[] },
+ ) => {
+ if ('databaseColumns' in obj && obj.databaseColumns) {
+ setDatabaseColumns(obj.databaseColumns);
+ }
+ if ('calculatedColumns' in obj && obj.calculatedColumns) {
+ setCalculatedColumns(obj.calculatedColumns);
+ }
+ },
+ [],
+ );
- this.props.addDangerToast(
- clientError ||
- statusText ||
- t('An error occurred while formatting SQL'),
- );
- } finally {
- this.abortControllers.formatQuery = null;
+ // Effect to trigger validation after user-initiated column changes
+ // Skips initial mount and prop-sync updates (which don't need validation)
+ useEffect(() => {
+ if (isInitialMount.current || isSyncingColumnsFromProps.current) {
+ isSyncingColumnsFromProps.current = false;
+ return;
}
- }
+ if (isComponentMounted.current) {
+ validateAndChange();
+ }
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [databaseColumns, calculatedColumns]);
- getSQLLabUrl() {
+ const getSQLLabUrl = useCallback(() => {
const queryParams = new URLSearchParams({
- dbid: String(this.state.datasource.database?.id ?? ''),
- sql: this.state.datasource.sql ?? '',
- name: this.state.datasource.datasource_name ?? '',
- schema: this.state.datasource.schema ?? '',
+ dbid: String(datasource.database?.id ?? ''),
+ sql: datasource.sql ?? '',
+ name: datasource.datasource_name ?? '',
+ schema: datasource.schema ?? '',
autorun: 'true',
isDataset: 'true',
});
return makeUrl(`/sqllab/?${queryParams.toString()}`);
- }
+ }, [datasource]);
- openOnSqlLab() {
- window.open(this.getSQLLabUrl(), '_blank', 'noopener,noreferrer');
- }
+ const openOnSqlLab = useCallback(() => {
+ window.open(getSQLLabUrl(), '_blank', 'noopener,noreferrer');
+ }, [getSQLLabUrl]);
- tableChangeAndSyncMetadata() {
- this.validate(() => {
- this.syncMetadata();
- this.onChange();
+ const onQueryRun = useCallback(async () => {
+ const databaseId = datasource.database?.id;
+ const { sql } = datasource;
+ if (!databaseId || !sql) {
+ return;
+ }
+ runQuery({
+ client_id: database?.clientId,
+ database_id: databaseId,
+ runAsync: false,
+ catalog: datasource.catalog,
+ schema: datasource.schema,
+ sql,
+ tmp_table_name: '',
+ select_as_cta: false,
+ ctas_method: 'TABLE',
+ queryLimit: 25,
+ expand_data: true,
});
- }
+ }, [datasource, database?.clientId, runQuery]);
- /**
- * Formats SQL query using the SQL format API endpoint.
- * Aborts any pending format requests before starting a new one.
- */
- async formatSql() {
- const { datasource } = this.state;
- if (!datasource.sql) {
+ const onQueryFormat = useCallback(async () => {
+ if (!datasource.sql || !isEditMode) {
return;
}
- // Abort previous formatSql if still pending
- if (this.abortControllers.formatSql) {
- this.abortControllers.formatSql.abort();
+ // Abort previous formatQuery if still pending
+ if (abortControllers.current.formatQuery) {
+ abortControllers.current.formatQuery.abort();
}
- this.abortControllers.formatSql = new AbortController();
- const { signal } = this.abortControllers.formatSql;
+ abortControllers.current.formatQuery = new AbortController();
+ const { signal } = abortControllers.current.formatQuery;
try {
- const response = await SupersetClient.post({
- endpoint: '/api/v1/sql/format',
- body: JSON.stringify({ sql: datasource.sql }),
- headers: { 'Content-Type': 'application/json' },
- signal,
- });
+ const response = await formatQueryAction(datasource.sql, { signal });
- this.onDatasourcePropChange('sql', response.json.result);
- this.props.addSuccessToast(t('SQL was formatted'));
- } catch (error) {
- if (error.name === 'AbortError') return;
+ onDatasourcePropChange('sql', response.json.result);
+ addSuccessToast(t('SQL was formatted'));
+ } catch (error: unknown) {
+ if ((error as Error).name === 'AbortError') return;
- const { error: clientError, statusText } =
- await getClientErrorObject(error);
+ const { error: clientError, statusText } = await getClientErrorObject(
+ error as Response,
+ );
- this.props.addDangerToast(
+ addDangerToast(
clientError ||
statusText ||
t('An error occurred while formatting SQL'),
);
} finally {
- this.abortControllers.formatSql = null;
+ abortControllers.current.formatQuery = null;
}
- }
-
- /**
- * Syncs dataset columns with the database schema.
- * Fetches column metadata from the underlying table/view and updates the
dataset.
- * Aborts any pending sync requests before starting a new one.
- */
- async syncMetadata() {
- const { datasource } = this.state;
-
+ }, [
+ datasource.sql,
+ isEditMode,
+ formatQueryAction,
+ onDatasourcePropChange,
+ addSuccessToast,
+ addDangerToast,
+ ]);
+
+ const syncMetadata = useCallback(async () => {
// Abort previous syncMetadata if still pending
- if (this.abortControllers.syncMetadata) {
- this.abortControllers.syncMetadata.abort();
+ if (abortControllers.current.syncMetadata) {
+ abortControllers.current.syncMetadata.abort();
}
- this.abortControllers.syncMetadata = new AbortController();
- const { signal } = this.abortControllers.syncMetadata;
+ abortControllers.current.syncMetadata = new AbortController();
+ const { signal } = abortControllers.current.syncMetadata;
- this.setState({ metadataLoading: true });
+ setMetadataLoading(true);
try {
const newCols = await fetchSyncedColumns(datasource, signal);
const columnChanges = updateColumns(
datasource.columns,
newCols,
- this.props.addSuccessToast,
+ addSuccessToast,
);
- this.setColumns({
+ setColumns({
databaseColumns: columnChanges.finalColumns.filter(
- col => !col.expression, // remove calculated columns
+ col => !col.expression,
) as Column[],
});
if (datasource.id !== undefined) {
clearDatasetCache(datasource.id);
}
- this.props.addSuccessToast(t('Metadata has been synced'));
- this.setState({ metadataLoading: false });
- } catch (error) {
- if (error.name === 'AbortError') {
- // Only update state if still mounted (abort may happen during unmount)
- if (this.isComponentMounted) {
- this.setState({ metadataLoading: false });
+ addSuccessToast(t('Metadata has been synced'));
+ setMetadataLoading(false);
+ } catch (error: unknown) {
+ if ((error as Error).name === 'AbortError') {
+ if (isComponentMounted.current) {
+ setMetadataLoading(false);
}
return;
}
- const { error: clientError, statusText } =
- await getClientErrorObject(error);
-
- this.props.addDangerToast(
- clientError || statusText || t('An error has occurred'),
+ const { error: clientError, statusText } = await getClientErrorObject(
+ error as Response,
);
- this.setState({ metadataLoading: false });
+
+ addDangerToast(clientError || statusText || t('An error has occurred'));
+ setMetadataLoading(false);
} finally {
- this.abortControllers.syncMetadata = null;
+ abortControllers.current.syncMetadata = null;
}
- }
+ }, [datasource, addSuccessToast, addDangerToast, setColumns]);
+
+ const fetchUsageData = useCallback(
+ async (
+ page = 1,
+ pageSize = 25,
+ sortColumn = 'changed_on_delta_humanized',
+ sortDirection = 'desc',
+ ) => {
+ // Abort previous fetchUsageData if still pending
+ if (abortControllers.current.fetchUsageData) {
+ abortControllers.current.fetchUsageData.abort();
+ }
- /**
- * Fetches chart usage data for this dataset (which charts use this dataset).
- * Aborts any pending fetch requests before starting a new one.
- *
- * @param {number} page - Page number (1-indexed)
- * @param {number} pageSize - Number of results per page
- * @param {string} sortColumn - Column to sort by
- * @param {string} sortDirection - Sort direction ('asc' or 'desc')
- * @returns {Promise<{charts: Array, count: number, ids: Array}>} Chart
usage data
- */
- async fetchUsageData(
- page = 1,
- pageSize = 25,
- sortColumn = 'changed_on_delta_humanized',
- sortDirection = 'desc',
- ) {
- const { datasource } = this.state;
-
- // Abort previous fetchUsageData if still pending
- if (this.abortControllers.fetchUsageData) {
- this.abortControllers.fetchUsageData.abort();
- }
+ abortControllers.current.fetchUsageData = new AbortController();
+ const { signal } = abortControllers.current.fetchUsageData;
+
+ try {
+ const queryParams = rison.encode({
+ columns: [
+ 'slice_name',
+ 'url',
+ 'certified_by',
+ 'certification_details',
+ 'description',
+ 'owners.first_name',
+ 'owners.last_name',
+ 'owners.id',
+ 'changed_on_delta_humanized',
+ 'changed_on',
+ 'changed_by.first_name',
+ 'changed_by.last_name',
+ 'changed_by.id',
+ 'dashboards.id',
+ 'dashboards.dashboard_title',
+ 'dashboards.url',
+ ],
+ filters: [
+ {
+ col: 'datasource_id',
+ opr: 'eq',
+ value: datasource.id,
+ },
+ ],
+ order_column: sortColumn,
+ order_direction: sortDirection,
+ page: page - 1,
+ page_size: pageSize,
+ });
+
+ const { json = {} } = await SupersetClient.get({
+ endpoint: `/api/v1/chart/?q=${queryParams}`,
+ signal,
+ });
- this.abortControllers.fetchUsageData = new AbortController();
- const { signal } = this.abortControllers.fetchUsageData;
+ const charts = json?.result || [];
+ const ids = json?.ids || [];
- try {
- const queryParams = rison.encode({
- columns: [
- 'slice_name',
- 'url',
- 'certified_by',
- 'certification_details',
- 'description',
- 'owners.first_name',
- 'owners.last_name',
- 'owners.id',
- 'changed_on_delta_humanized',
- 'changed_on',
- 'changed_by.first_name',
- 'changed_by.last_name',
- 'changed_by.id',
- 'dashboards.id',
- 'dashboards.dashboard_title',
- 'dashboards.url',
- ],
- filters: [
- {
- col: 'datasource_id',
- opr: 'eq',
- value: datasource.id,
- },
- ],
- order_column: sortColumn,
- order_direction: sortDirection,
- page: page - 1,
- page_size: pageSize,
- });
+ const chartsWithIds = charts.map(
+ (chart: Omit<ChartUsageData, 'id'>, index: number) => ({
+ ...chart,
+ id: ids[index],
+ }),
+ );
- const { json = {} } = await SupersetClient.get({
- endpoint: `/api/v1/chart/?q=${queryParams}`,
- signal,
- });
+ if (!signal.aborted && isComponentMounted.current) {
+ setUsageCharts(chartsWithIds);
+ setUsageChartsCount(json?.count || 0);
+ }
- const charts = json?.result || [];
- const ids = json?.ids || [];
+ return {
+ charts: chartsWithIds,
+ count: json?.count || 0,
+ ids,
+ };
+ } catch (error: unknown) {
+ if ((error as Error).name === 'AbortError') throw error;
- // Map chart IDs to chart objects
- const chartsWithIds = charts.map(
- (chart: Omit<ChartUsageData, 'id'>, index: number) => ({
- ...chart,
- id: ids[index],
- }),
- );
+ const { error: clientError, statusText } = await getClientErrorObject(
+ error as Response,
+ );
- // Only update state if not aborted and component still mounted
- if (!signal.aborted && this.isComponentMounted) {
- this.setState({
- usageCharts: chartsWithIds,
- usageChartsCount: json?.count || 0,
- });
+ addDangerToast(
+ clientError ||
+ statusText ||
+ t('An error occurred while fetching usage data'),
+ );
+ setUsageCharts([]);
+ setUsageChartsCount(0);
+
+ return {
+ charts: [],
+ count: 0,
+ ids: [],
+ };
+ } finally {
+ abortControllers.current.fetchUsageData = null;
}
+ },
+ [datasource.id, addDangerToast],
+ );
- return {
- charts: chartsWithIds,
- count: json?.count || 0,
- ids,
- };
- } catch (error) {
- // Rethrow AbortError so callers can handle gracefully
- if (error.name === 'AbortError') throw error;
+ const handleTabSelect = useCallback((key: string) => {
+ setActiveTabKey(key);
+ }, []);
- const { error: clientError, statusText } =
- await getClientErrorObject(error);
+ const sortMetrics = useCallback(
+ (metrics: Metric[]) =>
+ [...metrics].sort(
+ ({ id: a }: { id?: number }, { id: b }: { id?: number }) =>
+ (b ?? 0) - (a ?? 0),
+ ),
+ [],
+ );
- this.props.addDangerToast(
- clientError ||
- statusText ||
- t('An error occurred while fetching usage data'),
- );
- this.setState({
- usageCharts: [],
- usageChartsCount: 0,
+ // componentDidMount
+ useEffect(() => {
+ isComponentMounted.current = true;
+ // Mark initial mount as complete after first render cycle
+ // This prevents useEffect hooks from firing on mount
+ isInitialMount.current = false;
+ Mousetrap.bind('ctrl+shift+f', e => {
+ e.preventDefault();
+ if (isEditMode) {
+ onQueryFormat();
+ }
+ return false;
+ });
+ fetchUsageData().catch(error => {
+ if (error?.name !== 'AbortError') throw error;
+ });
+
+ // componentWillUnmount
+ return () => {
+ isComponentMounted.current = false;
+
+ // Abort all pending requests
+ Object.values(abortControllers.current).forEach(controller => {
+ if (controller) controller.abort();
});
- return {
- charts: [],
- count: 0,
- ids: [],
- };
- } finally {
- this.abortControllers.fetchUsageData = null;
- }
- }
+ Mousetrap.unbind('ctrl+shift+f');
+ resetQuery();
+ };
+ }, []);
- findDuplicates<T>(arr: T[], accessor: (obj: T) => string): string[] {
- const seen: Record<string, null> = {};
- const dups: string[] = [];
- arr.forEach((obj: T) => {
- const item = accessor(obj);
- if (item in seen) {
- dups.push(item);
- } else {
- seen[item] = null;
+ // Update Mousetrap binding when isEditMode changes
+ useEffect(() => {
+ Mousetrap.unbind('ctrl+shift+f');
+ Mousetrap.bind('ctrl+shift+f', e => {
+ e.preventDefault();
+ if (isEditMode) {
+ onQueryFormat();
}
+ return false;
});
- return dups;
- }
+ }, [isEditMode, onQueryFormat]);
Review Comment:
Done in c8a6059eaa — bound `ctrl+shift+f` once on mount and route through
`isEditModeRef` / `onQueryFormatRef` that a tiny per-render effect keeps
current. No more Mousetrap.unbind/bind on every SQL editor keystroke.
##########
superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.tsx:
##########
@@ -527,103 +363,215 @@ class AnnotationLayer extends PureComponent<
),
},
};
- this.setState({
- slice: dataObject,
- });
+ setSlice(dataObject);
});
- };
+ }, []);
- fetchAppliedChart(id: string | number): void {
- const { annotationType } = this.state;
- const registry = getChartMetadataRegistry();
- const queryParams = rison.encode({
- columns: ['slice_name', 'query_context', 'viz_type'],
- });
- SupersetClient.get({
- endpoint: `/api/v1/chart/${id}?q=${queryParams}`,
- }).then(({ json }) => {
- const { result } = json;
- const sliceName = result.slice_name;
- const queryContext = result.query_context;
- const vizType = result.viz_type;
- const formData = JSON.parse(queryContext).form_data;
- const metadata = registry.get(vizType);
- const canBeAnnotationType =
- metadata && metadata.canBeAnnotationType(annotationType);
- if (canBeAnnotationType) {
- this.setState({
- value: {
+ const fetchAppliedChart = useCallback(
+ (id: string | number): void => {
+ const registry = getChartMetadataRegistry();
+ const queryParams = rison.encode({
+ columns: ['slice_name', 'query_context', 'viz_type'],
+ });
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/${id}?q=${queryParams}`,
+ }).then(({ json }) => {
+ const { result } = json;
+ const sliceName = result.slice_name;
+ const queryContext = result.query_context;
+ const chartVizType = result.viz_type;
+ const formData = JSON.parse(queryContext).form_data;
+ const metadata = registry.get(chartVizType);
+ const canBeAnnotationType =
+ metadata && metadata.canBeAnnotationType(annotationType);
+ if (canBeAnnotationType) {
+ setValue({
value: id,
label: sliceName,
- },
- slice: {
+ });
+ setSlice({
data: {
...formData,
groupby: formData.groupby?.map((column: QueryFormColumn) =>
getColumnLabel(column),
),
},
- },
- });
- }
- });
- }
+ });
+ }
+ });
+ },
+ [annotationType],
+ );
- fetchAppliedNativeAnnotation(id: string | number): void {
- SupersetClient.get({
- endpoint: `/api/v1/annotation_layer/${id}`,
- }).then(({ json }) => {
- const { result } = json;
- const layer = result;
- this.setState({
- value: {
+ const fetchAppliedNativeAnnotation = useCallback(
+ (id: string | number): void => {
+ SupersetClient.get({
+ endpoint: `/api/v1/annotation_layer/${id}`,
+ }).then(({ json }) => {
+ const { result } = json;
+ const layer = result;
+ setValue({
value: layer.id,
label: layer.name,
- },
+ });
});
- });
- }
+ },
+ [],
+ );
+
+ const fetchAppliedAnnotation = useCallback(
+ (id: string | number): void => {
+ if (sourceType === ANNOTATION_SOURCE_TYPES.NATIVE) {
+ fetchAppliedNativeAnnotation(id);
+ } else {
+ fetchAppliedChart(id);
+ }
+ },
+ [sourceType, fetchAppliedNativeAnnotation, fetchAppliedChart],
+ );
- fetchAppliedAnnotation(id: string | number): void {
- const { sourceType } = this.state;
+ // componentDidMount - fetch applied annotation if needed
+ useEffect(() => {
+ if (shouldFetchAppliedAnnotation()) {
+ /* The value prop is the id of the chart/native. This function will set
+ value in state to an object with the id as value.value to be used by
+ AsyncSelect */
+ if (
+ propValue !== null &&
+ propValue !== undefined &&
+ typeof propValue !== 'object'
+ ) {
+ fetchAppliedAnnotation(propValue);
+ }
+ }
+ // Only run on mount
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, []);
+
+ // Track previous value for componentDidUpdate comparison
+ const [prevValue, setPrevValue] = useState<
+ string | number | SelectOption | null
+ >(value);
- if (sourceType === ANNOTATION_SOURCE_TYPES.NATIVE) {
- return this.fetchAppliedNativeAnnotation(id);
+ // componentDidUpdate - fetch slice data when value changes
+ useEffect(() => {
+ if (value !== prevValue) {
+ const isChart =
+ sourceType !== ANNOTATION_SOURCE_TYPES.NATIVE &&
+ requiresQuery(sourceType ?? undefined);
+ if (isChart && value && typeof value === 'object' && 'value' in value) {
+ fetchSliceData(value.value);
Review Comment:
Done in e8f20a67e4 — gated the value-change watcher's `fetchSliceData` on
`!slice`. `fetchAppliedChart` sets both `value` and `slice` synchronously, so
the gate now skips the redundant fetch. `handleSelectValue` clears `slice` to
null first, so the user-selection path still fires the watcher correctly for a
fresh chart.
##########
superset-frontend/src/explore/components/controls/TextAreaControl.tsx:
##########
@@ -74,207 +67,259 @@ interface TextAreaControlProps {
tooltipOptions?: Record<string, unknown>;
hotkeys?: HotkeyConfig[];
debounceDelay?: number | null;
- theme?: ThemeType;
'aria-required'?: boolean;
value?: string;
[key: string]: unknown;
}
-const defaultProps = {
- onChange: () => {},
- height: 250,
- minLines: 3,
- maxLines: 10,
- offerEditInModal: true,
- readOnly: false,
- resize: null,
- textAreaStyles: {},
- tooltipOptions: {},
- hotkeys: [],
- debounceDelay: null,
-};
-
-class TextAreaControl extends Component<TextAreaControlProps> {
- static defaultProps = defaultProps;
+function TextAreaControl({
+ name,
+ onChange = () => {},
+ initialValue,
+ height = 250,
+ minLines = 3,
+ maxLines = 10,
+ offerEditInModal = true,
+ language,
+ aboveEditorSection,
+ readOnly = false,
+ resize = null,
+ textAreaStyles = {},
+ tooltipOptions = {},
+ hotkeys = [],
+ debounceDelay = null,
+ 'aria-required': ariaRequired,
+ value,
+ ...restProps
+}: TextAreaControlProps) {
+ const theme = useTheme();
- debouncedOnChange:
- | ReturnType<typeof debounce<(value: string) => void>>
- | undefined;
+ const debouncedOnChangeRef = useRef<ReturnType<
+ typeof debounce<(value: string) => void>
+ > | null>(null);
- constructor(props: TextAreaControlProps) {
- super(props);
- if (props.debounceDelay && props.onChange) {
- this.debouncedOnChange = debounce(props.onChange, props.debounceDelay);
- }
- }
-
- componentDidUpdate(prevProps: TextAreaControlProps) {
- if (
- this.props.onChange !== prevProps.onChange &&
- this.props.debounceDelay &&
- this.props.onChange
- ) {
- if (this.debouncedOnChange) {
- this.debouncedOnChange.cancel();
+ // Create or update debounced onChange when dependencies change
+ useEffect(() => {
+ if (debounceDelay && onChange) {
+ if (debouncedOnChangeRef.current) {
+ debouncedOnChangeRef.current.cancel();
}
- this.debouncedOnChange = debounce(
- this.props.onChange,
- this.props.debounceDelay,
- );
- }
- }
-
- handleChange(value: string | { target: { value: string } }) {
- const finalValue = typeof value === 'object' ? value.target.value : value;
- if (this.debouncedOnChange) {
- this.debouncedOnChange(finalValue);
+ debouncedOnChangeRef.current = debounce(onChange, debounceDelay);
} else {
- this.props.onChange?.(finalValue);
- }
- }
-
- componentWillUnmount() {
- if (this.debouncedOnChange) {
- this.debouncedOnChange.flush();
+ if (debouncedOnChangeRef.current) {
+ debouncedOnChangeRef.current.cancel();
+ }
+ debouncedOnChangeRef.current = null;
}
- }
+ }, [onChange, debounceDelay]);
- renderEditor(inModal = false) {
- // Exclude props that shouldn't be passed to TextAreaEditor:
- // - theme: TextAreaEditor expects theme as a string, not the theme object
from withTheme HOC
- // - height: ReactAce expects string, we pass number (height is controlled
via minLines/maxLines)
- // - other control-specific props and explicitly-set props to avoid
duplicate/conflicting assignments
- const {
- theme,
- height,
- offerEditInModal,
- aboveEditorSection,
- resize,
- textAreaStyles,
- tooltipOptions,
- hotkeys,
- debounceDelay,
- language,
- initialValue,
- readOnly,
- name,
- onChange,
- value,
- minLines: minLinesProp,
- maxLines: maxLinesProp,
- ...editorProps
- } = this.props;
- const minLines = inModal ? 40 : minLinesProp || 12;
- if (language) {
- const style: React.CSSProperties = {
- border: theme?.colorBorder
- ? `1px solid ${theme.colorBorder}`
- : undefined,
- minHeight: `${minLines}em`,
- width: 'auto',
- ...textAreaStyles,
- };
- if (resize) {
- style.resize = resize;
- style.overflow = 'auto';
+ // Cleanup on unmount — flush pending debounced onChange so last edit isn't
lost
+ useEffect(
+ () => () => {
+ if (debouncedOnChangeRef.current) {
+ debouncedOnChangeRef.current.flush();
}
- if (readOnly) {
- style.backgroundColor = theme?.colorBgMask;
+ },
+ [],
+ );
+
+ const handleChange = useCallback(
+ (val: string | { target: { value: string } }) => {
+ const finalValue = typeof val === 'object' ? val.target.value : val;
+ if (debouncedOnChangeRef.current) {
+ debouncedOnChangeRef.current(finalValue);
+ } else {
+ onChange?.(finalValue);
}
- const onEditorLoad = (editor: {
- commands: {
- addCommand: (cmd: {
- name: string;
- bindKey: { win: string; mac: string };
- exec: () => void;
- }) => void;
- };
- }) => {
- hotkeys?.forEach(keyConfig => {
- editor.commands.addCommand({
- name: keyConfig.name,
- bindKey: { win: keyConfig.key, mac: keyConfig.key },
- exec: keyConfig.func,
- });
- });
+ },
+ [onChange],
+ );
+
+ const onEditorLoad = useCallback(
+ (editor: {
+ commands: {
+ addCommand: (cmd: {
+ name: string;
+ bindKey: { win: string; mac: string };
+ exec: () => void;
+ }) => void;
};
- const codeEditor = (
+ }) => {
+ hotkeys?.forEach(keyConfig => {
+ editor.commands.addCommand({
+ name: keyConfig.name,
+ bindKey: { win: keyConfig.key, mac: keyConfig.key },
+ exec: keyConfig.func,
+ });
+ });
+ },
+ [hotkeys],
+ );
+
+ const renderEditor = useCallback(
+ (inModal = false) => {
+ const effectiveMinLines = inModal ? 40 : minLines || 12;
+
+ if (language) {
+ const style: React.CSSProperties = {
+ border: theme?.colorBorder
+ ? `1px solid ${theme.colorBorder}`
+ : undefined,
+ minHeight: `${effectiveMinLines}em`,
+ width: 'auto',
+ ...textAreaStyles,
+ };
+
+ if (resize) {
+ style.resize = resize;
+ style.overflow = 'auto';
+ }
+
+ if (readOnly) {
+ style.backgroundColor = theme?.colorBgMask;
+ }
+
+ const codeEditor = (
+ <div>
+ <TextAreaEditor
+ mode={language}
+ style={style}
+ minLines={effectiveMinLines}
+ maxLines={inModal ? 1000 : maxLines}
+ editorProps={{ $blockScrolling: true }}
+ onLoad={onEditorLoad}
+ defaultValue={initialValue ?? value}
+ readOnly={readOnly}
+ key={name}
+ {...restProps}
+ onChange={handleChange}
+ />
+ </div>
+ );
+
+ if (tooltipOptions && Object.keys(tooltipOptions).length > 0) {
+ return <Tooltip {...tooltipOptions}>{codeEditor}</Tooltip>;
+ }
+ return codeEditor;
+ }
+
+ const textArea = (
<div>
- <TextAreaEditor
- mode={language}
- style={style}
- minLines={minLines}
- maxLines={inModal ? 1000 : maxLinesProp}
- editorProps={{ $blockScrolling: true }}
- onLoad={onEditorLoad}
+ <Input.TextArea
+ placeholder={t('textarea')}
+ onChange={handleChange}
defaultValue={initialValue ?? value}
- readOnly={readOnly}
- key={name}
- {...editorProps}
- onChange={this.handleChange.bind(this)}
+ disabled={readOnly}
+ style={{ height }}
+ aria-required={ariaRequired}
/>
</div>
);
- if (tooltipOptions) {
- return <Tooltip {...tooltipOptions}>{codeEditor}</Tooltip>;
+ if (tooltipOptions && Object.keys(tooltipOptions).length > 0) {
+ return <Tooltip {...tooltipOptions}>{textArea}</Tooltip>;
}
- return codeEditor;
- }
+ return textArea;
+ },
+ [
+ minLines,
+ maxLines,
+ language,
+ theme,
+ textAreaStyles,
+ resize,
+ readOnly,
+ onEditorLoad,
+ initialValue,
+ value,
+ name,
+ restProps,
+ handleChange,
+ tooltipOptions,
+ height,
+ ariaRequired,
+ ],
+ );
- const textArea = (
- <div>
- <Input.TextArea
- placeholder={t('textarea')}
- onChange={this.handleChange.bind(this)}
- defaultValue={this.props.initialValue}
- disabled={this.props.readOnly}
- style={{ height: this.props.height }}
- aria-required={this.props['aria-required']}
- />
- </div>
- );
- if (this.props.tooltipOptions) {
- return <Tooltip {...this.props.tooltipOptions}>{textArea}</Tooltip>;
- }
- return textArea;
- }
+ // Extract only ControlHeader-compatible props from restProps
+ const {
+ label,
+ description,
+ validationErrors,
+ renderTrigger,
+ rightNode,
+ leftNode,
+ onClick,
+ hovered,
+ tooltipOnClick,
+ warning,
+ danger,
+ } = restProps as Record<string, unknown>;
+
+ const controlHeader = useMemo(
Review Comment:
Done in 62808305f8 — same commit as the modalTitle widening. Replaced the
12-key destructure-and-cast with `<ControlHeader name={name} {...restProps}
/>`, matching the `ViewportControl` spread style.
##########
superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.tsx:
##########
@@ -640,33 +588,53 @@ class AnnotationLayer extends PureComponent<
newAnnotation.color = null;
}
- this.props.addAnnotationLayer?.(newAnnotation);
- this.setState({ isNew: false });
+ addAnnotationLayer?.(newAnnotation);
+ setIsNew(false);
}
- }
+ }, [
+ isValidForm,
+ name,
+ annotationType,
+ sourceType,
+ color,
+ opacity,
+ style,
+ width,
+ showMarkers,
+ hideLine,
+ overrides,
+ show,
+ showLabel,
+ titleColumn,
+ descriptionColumns,
+ timeColumn,
+ intervalEndColumn,
+ value,
+ addAnnotationLayer,
+ ]);
- submitAnnotation(): void {
- this.applyAnnotation();
- this.props.close?.();
- }
+ const submitAnnotation = useCallback((): void => {
+ applyAnnotation();
+ close?.();
+ }, [applyAnnotation, close]);
- renderChartHeader(
- label: string,
- description: string,
- value: string | number | SelectOption | null,
- ): React.ReactNode {
- return (
+ const renderChartHeader = useCallback(
Review Comment:
Partially done in e8f20a67e4 — inlined `renderChartHeader` (the clearest
dead-useCallback case) as a plain `buildChartHeader` helper. There are a few
more `useCallback`s in this file with no memoized downstream consumers; happy
to audit them all in a follow-up if you want, but I wanted to keep this PR's
diff bounded around the conversion + the load-bearing fixes you flagged.
##########
superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx:
##########
@@ -46,6 +46,8 @@
jest.mock('src/components/Datasource/components/DatasourceEditor', () => ({
),
}));
+const SupersetClientGet = jest.spyOn(SupersetClient, 'get');
Review Comment:
Done in 62808305f8 — added an `afterAll` that calls
`SupersetClientGet.mockRestore()` so the module-scope spy doesn't leak into
other test files in the same Jest worker.
##########
superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx:
##########
@@ -54,19 +56,8 @@ beforeEach(() => {
afterEach(() => {
window.location = originalLocation;
-
- try {
- const unmatched = fetchMock.callHistory.calls('unmatched');
- if (unmatched.length > 0) {
- const urls = unmatched.map(call => call.url).join(', ');
- throw new Error(
- `fetchMock: ${unmatched.length} unmatched call(s): ${urls}`,
- );
- }
- } finally {
- fetchMock.clearHistory().removeRoutes();
- jest.restoreAllMocks();
- }
+ fetchMock.clearHistory().removeRoutes();
+ jest.clearAllMocks(); // Clears mock history but keeps spy in place
});
Review Comment:
Same fix as the line-49 thread — afterAll mockRestore in 62808305f8.
##########
superset-frontend/src/explore/components/SaveModal.test.tsx:
##########
@@ -330,139 +329,43 @@ test('renders InfoTooltip icon next to Dataset Name
label when datasource type i
expect(labelContainer).toContainElement(infoTooltip);
});
-test('make sure slice_id in the URLSearchParams before the redirect', () => {
- const myProps = {
- ...defaultProps,
- slice: { slice_id: 1, slice_name: 'title', owners: [1] },
- actions: {
- setFormData: jest.fn(),
- updateSlice: jest.fn(() => Promise.resolve({ id: 1 })),
- getSliceDashboards: jest.fn(),
- },
- user: { userId: 1 },
- history: {
- replace: jest.fn(),
- },
- dispatch: jest.fn(),
- };
-
- const saveModal = new TestSaveModal(myProps);
- const result = saveModal.handleRedirect(
- 'https://example.com/?name=John&age=30',
+test('createRedirectParams sets slice_id in the URLSearchParams', () => {
+ const result = createRedirectParams(
+ '?name=John&age=30',
{ id: 1 },
+ 'overwrite',
);
expect(result.get('slice_id')).toEqual('1');
+ expect(result.get('save_action')).toEqual('overwrite');
});
-test('removes form_data_key from URL parameters after save', () => {
- const myProps = {
- ...defaultProps,
- slice: { slice_id: 1, slice_name: 'title', owners: [1] },
- actions: {
- setFormData: jest.fn(),
- updateSlice: jest.fn(() => Promise.resolve({ id: 1 })),
- getSliceDashboards: jest.fn(),
- },
- user: { userId: 1 },
- history: {
- replace: jest.fn(),
- },
- dispatch: jest.fn(),
- };
-
- const saveModal = new TestSaveModal(myProps);
-
+test('createRedirectParams removes form_data_key from URL parameters', () => {
// Test with form_data_key in the URL
const urlWithFormDataKey = '?form_data_key=12345&other_param=value';
- const result = saveModal.handleRedirect(urlWithFormDataKey, { id: 1 });
+ const result = createRedirectParams(
+ urlWithFormDataKey,
+ { id: 1 },
+ 'overwrite',
+ );
// form_data_key should be removed
expect(result.has('form_data_key')).toBe(false);
// other parameters should remain
expect(result.get('other_param')).toEqual('value');
expect(result.get('slice_id')).toEqual('1');
- expect(result.has('save_action')).toBe(false);
+ expect(result.get('save_action')).toEqual('overwrite');
});
-test('dispatches removeChartState when saving and going to dashboard', async
() => {
- // Spy on the removeChartState action creator
- const removeChartStateSpy = jest.spyOn(
- dashboardStateActions,
- 'removeChartState',
- );
-
- // Mock the dashboard API response
- const dashboardId = 123;
- const dashboardUrl = '/superset/dashboard/test-dashboard/';
- fetchMock.get(`glob:*/api/v1/dashboard/${dashboardId}*`, {
- result: {
- id: dashboardId,
- dashboard_title: 'Test Dashboard',
- url: dashboardUrl,
- },
- });
-
- const mockDispatch = jest.fn();
- const mockHistory = {
- push: jest.fn(),
- replace: jest.fn(),
- };
- const chartId = 42;
- const mockUpdateSlice = jest.fn(() => Promise.resolve({ id: chartId }));
- const mockSetFormData = jest.fn();
-
- const myProps = {
- ...defaultProps,
- slice: { slice_id: 1, slice_name: 'title', owners: [1] },
- actions: {
- setFormData: mockSetFormData,
- updateSlice: mockUpdateSlice,
- getSliceDashboards: jest.fn(() => Promise.resolve([])),
- saveSliceFailed: jest.fn(),
- },
- user: { userId: 1 },
- history: mockHistory,
- dispatch: mockDispatch,
- };
-
- const saveModal = new TestSaveModal(myProps);
- saveModal.state = {
- action: 'overwrite',
- newSliceName: 'test chart',
- datasetName: 'test dataset',
- dashboard: { label: 'Test Dashboard', value: dashboardId },
- saveStatus: null,
- isLoading: false,
- tabsData: [],
- };
-
- // Mock onHide to prevent errors
- saveModal.onHide = jest.fn();
-
- // Trigger save and go to dashboard (gotodash = true)
- await saveModal.saveOrOverwrite(true);
-
- // Wait for async operations
- await waitFor(() => {
- expect(mockUpdateSlice).toHaveBeenCalled();
- expect(mockSetFormData).toHaveBeenCalled();
- });
-
- // Verify removeChartState was called with the correct chart ID
- expect(removeChartStateSpy).toHaveBeenCalledWith(chartId);
-
- // Verify the action was dispatched (check the action object directly)
- expect(mockDispatch).toHaveBeenCalled();
- expect(mockDispatch).toHaveBeenCalledWith({
- type: 'REMOVE_CHART_STATE',
- chartId,
- });
-
- // Verify navigation happened
- expect(mockHistory.push).toHaveBeenCalled();
-
- // Clean up
- removeChartStateSpy.mockRestore();
+/**
+ * TODO: This test was written for the class component version of SaveModal.
+ * Since SaveModal has been converted to a function component, this test
+ * needs to be rewritten to test through component rendering and user
interaction.
+ * The test should verify that clicking "Save & go to dashboard" dispatches
+ * removeChartState with the correct chart ID.
+ */
+test('dispatches removeChartState when saving and going to dashboard -
placeholder', () => {
+ // See TODO comment above
+ expect(true).toBe(true);
});
Review Comment:
Removed the placeholders in a396b654d rather than rewriting them inline —
they were left over from the class version and were net-negative as regression
guards. FC-shaped replacements (`history.replace({ saveAction })`, `OUT_OF_TAB`
strip, `onDashboardChange(null)` guard) are tracked in @sadpandajoe's other
threads on this PR and will land in a focused follow-up rather than bloating
the conversion diff.
--
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]
