From 0879c8cddc7fe0e9983188cf5d4107af2fce3d0b Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Wed, 11 Feb 2026 09:33:32 -0800 Subject: [PATCH] fix: address code review comments from bot - ChartDataProvider: fix useEffect to only refetch on formData/sliceId changes - reactify: preserve legacy `this` context for componentWillUnmount callbacks - HorizonRow: add empty array guard for colorScale="change" - TTestTable: clamp control index when data shrinks - TableRenderers: fix sorting state reset to only trigger on structural props - Chart: initialize renderStartTimeRef with Logger.getTimestamp() - DatasourceEditor: pass fresh validation errors to onChange callback - Dashboard: use event parameter instead of window.event in beforeunload - SliceAdder: use refs to track latest values in cleanup effect - Markdown: add ErrorBoundary and error handler to enable error message - SaveModal: add isLoading check to "Save & go to dashboard" button - CollectionControl: forward header props to ControlHeader Co-Authored-By: Claude Opus 4.5 --- .../chart/components/ChartDataProvider.tsx | 6 ++- .../src/chart/components/reactify.tsx | 4 +- .../src/HorizonRow.tsx | 2 +- .../src/TTestTable.tsx | 19 ++++++-- .../src/react-pivottable/TableRenderers.tsx | 12 +++++- .../src/components/Chart/Chart.tsx | 2 +- .../DatasourceEditor/DatasourceEditor.tsx | 15 ++++--- .../src/dashboard/components/Dashboard.tsx | 6 +-- .../src/dashboard/components/SliceAdder.tsx | 23 +++++++--- .../gridComponents/Markdown/Markdown.tsx | 43 ++++++++++++++----- .../src/explore/components/SaveModal.tsx | 1 + .../controls/CollectionControl/index.tsx | 6 ++- 12 files changed, 103 insertions(+), 36 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/ChartDataProvider.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/ChartDataProvider.tsx index a7776fbf0dc..1a17db7a884 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/ChartDataProvider.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/ChartDataProvider.tsx @@ -146,9 +146,13 @@ function ChartDataProvider({ ]); // Fetch data on mount and when formData or sliceId changes + // Note: handleFetchData depends on callback props, so changing callbacks + // will also trigger a refetch. This mirrors the original class behavior + // where componentDidMount always fetched. useEffect(() => { handleFetchData(); - }, [formData, sliceId, handleFetchData]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [formData, sliceId]); const { status, payload, error } = state; diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/reactify.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/reactify.tsx index 75636b7b2ff..3e94e6c6583 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/reactify.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/reactify.tsx @@ -97,7 +97,9 @@ export default function reactify( useEffect( () => () => { if (callbacks?.componentWillUnmount) { - callbacks.componentWillUnmount(); + // Preserve legacy behavior where `this` was a component instance + // exposing a `container` property + callbacks.componentWillUnmount.call({ container: containerRef.current }); } }, [], diff --git a/superset-frontend/plugins/legacy-plugin-chart-horizon/src/HorizonRow.tsx b/superset-frontend/plugins/legacy-plugin-chart-horizon/src/HorizonRow.tsx index 7e988865fdc..5431674b7e9 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-horizon/src/HorizonRow.tsx +++ b/superset-frontend/plugins/legacy-plugin-chart-horizon/src/HorizonRow.tsx @@ -70,7 +70,7 @@ function HorizonRow({ if (!canvas) return; const data = - colorScale === 'change' + colorScale === 'change' && rawData.length > 0 ? rawData.map(d => ({ ...d, y: d.y - rawData[0].y })) : rawData; diff --git a/superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx b/superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx index 36210009277..47d07636a7c 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx +++ b/superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx @@ -134,10 +134,23 @@ function TTestTable({ [data, computeLift, computePValue], ); - // Initially populate table on mount + // Recompute table when data or control row changes, keeping control index in range useEffect(() => { - computeTTest(control); - }, [computeTTest, control]); + if (!data || data.length === 0) { + setControl(0); + setLiftValues([]); + setPValues([]); + return; + } + + const safeControlIndex = Math.min(control, data.length - 1); + if (safeControlIndex !== control) { + setControl(safeControlIndex); + computeTTest(safeControlIndex); + } else { + computeTTest(control); + } + }, [computeTTest, control, data]); const getLiftStatus = useCallback( (row: number): string => { diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx b/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx index c786dda7502..4be078c0986 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx @@ -709,12 +709,20 @@ export function TableRenderer({ return getBasePivotSettings(); }, [getBasePivotSettings]); - // Reset sort state when props change + // Reset sort state when structural props change useEffect(() => { setSortingOrder([]); setActiveSortColumn(null); setSortedRowKeys(null); - }, [props]); + }, [ + cols, + rows, + aggregatorName, + tableOptions, + subtotalOptions, + namesMappingProp, + allowRenderHtml, + ]); // Use sorted row keys if available, otherwise use base row keys const effectiveRowKeys = sortedRowKeys ?? basePivotSettings.rowKeys; diff --git a/superset-frontend/src/components/Chart/Chart.tsx b/superset-frontend/src/components/Chart/Chart.tsx index 37d4903f286..ac67ac7478a 100644 --- a/superset-frontend/src/components/Chart/Chart.tsx +++ b/superset-frontend/src/components/Chart/Chart.tsx @@ -212,7 +212,7 @@ function Chart({ suppressLoadingSpinner, } = restProps; - const renderStartTimeRef = useRef(0); + const renderStartTimeRef = useRef(Logger.getTimestamp()); const shouldRenderChart = useCallback( () => diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx index 7b38cc59b75..7603987b2c0 100644 --- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx @@ -934,7 +934,7 @@ function DatasourceEditor({ ); const validate = useCallback( - (callback: () => void) => { + (callback: (validationErrors: string[]) => void) => { let validationErrors: string[] = []; let dups: string[]; @@ -984,15 +984,16 @@ function DatasourceEditor({ } setErrors(validationErrors); - callback(); + callback(validationErrors); }, [datasource, calculatedColumns, folders, findDuplicates], ); - const onChangeInternal = useCallback(() => { - // Emptying SQL if "Physical" radio button is selected - const sql = - datasourceType === DATASOURCE_TYPES.physical.key ? '' : datasource.sql; + const onChangeInternal = useCallback( + (validationErrors: string[] = errors) => { + // Emptying SQL if "Physical" radio button is selected + const sql = + datasourceType === DATASOURCE_TYPES.physical.key ? '' : datasource.sql; const columns = [ ...databaseColumns, @@ -1016,7 +1017,7 @@ function DatasourceEditor({ folders: filteredFolders, }; - onChange(newDatasource, errors); + onChange(newDatasource, validationErrors); }, [ datasource, datasourceType, diff --git a/superset-frontend/src/dashboard/components/Dashboard.tsx b/superset-frontend/src/dashboard/components/Dashboard.tsx index 076c475b22b..710e837aa1d 100644 --- a/superset-frontend/src/dashboard/components/Dashboard.tsx +++ b/superset-frontend/src/dashboard/components/Dashboard.tsx @@ -98,10 +98,10 @@ function onBeforeUnload(hasChanged: boolean): void { } } -function unload(): string { +function unload(event: BeforeUnloadEvent): string { const message = t('You have unsaved changes.'); - // Gecko + IE: returnValue is typed as boolean but historically accepts string - (window.event as BeforeUnloadEvent).returnValue = message; + // Set returnValue on the actual event object to trigger the browser prompt + event.returnValue = message; return message; // Gecko + Webkit, Safari, Chrome etc. } diff --git a/superset-frontend/src/dashboard/components/SliceAdder.tsx b/superset-frontend/src/dashboard/components/SliceAdder.tsx index b06875603a3..b58d23b235c 100644 --- a/superset-frontend/src/dashboard/components/SliceAdder.tsx +++ b/superset-frontend/src/dashboard/components/SliceAdder.tsx @@ -183,10 +183,23 @@ function SliceAdder({ const [selectedSliceIdsSet, setSelectedSliceIdsSet] = useState( () => new Set(selectedSliceIds), ); + + // Refs to track latest values for cleanup effect + const latestSlicesRef = useRef(slices); + const latestSelectedSliceIdsSetRef = useRef(selectedSliceIdsSet); const [showOnlyMyCharts, setShowOnlyMyCharts] = useState(() => getItem(LocalStorageKeys.DashboardEditorShowOnlyMyCharts, true), ); + // Keep refs updated with latest values + useEffect(() => { + latestSlicesRef.current = slices; + }, [slices]); + + useEffect(() => { + latestSelectedSliceIdsSetRef.current = selectedSliceIdsSet; + }, [selectedSliceIdsSet]); + const filteredSlices = useMemo( () => getFilteredSortedSlices( @@ -220,8 +233,10 @@ function SliceAdder({ useEffect( () => () => { // Clears the redux store keeping only selected items - const selectedSlices = pickBy(slices, (value: Slice) => - selectedSliceIdsSet.has(value.slice_id), + // Use refs to get latest values on unmount + const selectedSlices = pickBy( + latestSlicesRef.current, + (value: Slice) => latestSelectedSliceIdsSetRef.current.has(value.slice_id), ); updateSlices(selectedSlices); @@ -229,9 +244,7 @@ function SliceAdder({ slicesRequestRef.current.abort(); } }, - // Only run on unmount - capture current values - // eslint-disable-next-line react-hooks/exhaustive-deps - [], + [updateSlices], ); const searchUpdated = useCallback((term: string) => { diff --git a/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx b/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx index 3639ed49e63..70350814180 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx @@ -21,6 +21,7 @@ import { connect } from 'react-redux'; import cx from 'classnames'; import type { JsonObject } from '@superset-ui/core'; import type { ResizeStartCallback, ResizeCallback } from 're-resizable'; +import { ErrorBoundary } from 'src/components'; import { t, css, styled } from '@apache-superset/core/ui'; import { SafeMarkdown } from '@superset-ui/core/components'; @@ -298,6 +299,20 @@ function Markdown({ [], ); + const handleRenderError = useCallback( + (error: Error, info: { componentStack: string } | null): void => { + setHasError(true); + if (editorMode === 'preview') { + addDangerToast( + t( + 'This markdown component has an error. Please revert your recent changes.', + ), + ); + } + }, + [addDangerToast, editorMode], + ); + const renderEditMode = useMemo( () => ( ( - + + + ), - [hasError, markdownSource, htmlSanitization, htmlSchemaOverrides], + [ + hasError, + markdownSource, + htmlSanitization, + htmlSchemaOverrides, + handleRenderError, + ], ); // inherit the size of parent columns diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index 7759c3dd346..b0c96830fda 100644 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -774,6 +774,7 @@ const SaveModal = ({ id="btn_modal_save_goto_dash" buttonSize="small" disabled={ + isLoading || !newSliceName || !dashboard || (datasource?.type !== DatasourceType.Table && !datasetName) diff --git a/superset-frontend/src/explore/components/controls/CollectionControl/index.tsx b/superset-frontend/src/explore/components/controls/CollectionControl/index.tsx index 2cb30fd68a4..433cf9a0f09 100644 --- a/superset-frontend/src/explore/components/controls/CollectionControl/index.tsx +++ b/superset-frontend/src/explore/components/controls/CollectionControl/index.tsx @@ -83,7 +83,8 @@ export default function CollectionControl({ isFloat, isInt, controlName, -}: CollectionControlProps) { + ...headerProps +}: CollectionControlProps & { [key: string]: unknown }) { const theme = useTheme(); const handleChange = useCallback( @@ -200,11 +201,12 @@ export default function CollectionControl({ ); }; - // Props for ControlHeader (excluding label and theme which are handled separately) + // Props for ControlHeader, including any header-related props passed from the parent const controlHeaderProps = { name, label, description, + ...headerProps, }; return (