rusackas commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3229488775
##########
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:
Good catch — that was an inadvertent narrowing. Master used
`modalTitle={controlHeader as any}` to bypass `ModalTrigger`'s `modalTitle?:
string` typing and pass the full `<ControlHeader>` (with description tooltip,
validation badges, warning icons). The FC version stripped the cast and fell
back to `String(label || '')`, which is a regression in the modal header UX.
The proper fix is to widen `ModalTrigger`'s `modalTitle?` to `string |
ReactNode` (and `title`/`name` on `StyledModal` for the inner `Modal`) and pass
`controlHeader` back in. Doing that in a follow-up since it touches a shared
component type.
##########
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:
Agreed. The class versions of `SelectControl`, `AnnotationLayerControl`,
`AdhocFilter*Control`, `AdhocMetric*Control`, and `FixedOrMetricControl` were
all `PureComponent`, so their skip-on-shallow-equal-props behavior is gone
after the conversion. I'll wrap each in `memo()` (most are pure with no
children, so the default shallow-equal works) in a follow-up commit. Leaving
thread open as a tracker.
##########
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:
Real perf issue. The `useCallback` for the keyboard handler has changing
deps so the registered `mousetrap` binding gets torn down and rebuilt on every
relevant state change. The fix is either to (a) move the binding into a
`useEffect` keyed off the stable identity and read the changing values via a
ref, or (b) bind the shortcut once via a top-level effect and route the action
through a ref-based callback. Will address in a follow-up.
##########
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:
Looking at it — `fetchAppliedChart(value)` calls `setSlice` (synchronous),
then this watcher fires `fetchSliceData(value.value)` which re-resolves the
same chart. Most likely an unintentional artifact of splitting what was a
single `componentDidUpdate` branch into multiple effects during the conversion.
The watcher should only fire when `slice` arrives from a *different* path
(e.g., direct slice select). Will narrow the dep / add an `if (slice.slice_id
!== value.value)` guard in a follow-up.
##########
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:
Fair point — the `ViewportControl`-style `<ControlHeader {...restProps} />`
is cleaner. The heavy destructure was a literal port of how the class accessed
`this.props` 12 times, but for a pure FC `{...restProps}` is equivalent and a
third the LOC. I'll align both controls on the spread approach in a follow-up.
##########
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:
Agreed — the cached identity isn't observed (no memoized child consumer), so
the dep-array comparison work is dead weight. The class-component conversion
mechanically replaced every `this.X = X.bind(this)` with a `useCallback`
without checking whether the stable identity was load-bearing downstream. I'll
inline these (and audit the other unjustified `useCallback`s in the same file)
in a follow-up.
--
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]
