sadpandajoe commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3191613141
##########
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:
SelectControl shows up many times in the explore control panel, so we should
wrap it with `memo()` to keep the same skip behavior as before. The other
PureComponent classes converted in this PR (AnnotationLayerControl,
AdhocFilter*, AdhocMetric*, FixedOrMetricControl) likely need the same.
##########
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:
Should we bind this once? Every keystroke recreates the callback and rebinds
`ctrl+shift+f`, which could be a performance issue.
##########
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:
This 12-prop destructure-and-cast block is heavier than `<ControlHeader
{...restProps} />` used in `ViewportControl` in this same PR. Should we align
on one approach?
##########
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:
`fetchAppliedChart` already calls `setSlice(...)` inline before this watcher
runs, so calling `fetchSliceData(value.value)` here re-fetches the same chart
and overwrites what we just set. Was that double-fetch intentional, or a side
effect of the conversion?
##########
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:
This used to render the full `<ControlHeader>` (description tooltip,
validation badges, warning icons). Did we mean to drop that from the modal
title bar?
##########
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:
Should we inline these instead? `useCallback` runs a dep-array comparison
every render but no downstream consumer is memoized, so we never benefit from
the cached identity.
--
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]
