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 <noreply@anthropic.com>

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;
 

superset-frontend/packages/superset-ui-core/src/chart/components/reactify.tsx

@@ -97,7 +97,9 @@ export default function reactify<Props extends object>(
     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 });
         }
       },
       [],

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;
 

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 => {

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;

superset-frontend/src/components/Chart/Chart.tsx

@@ -212,7 +212,7 @@ function Chart({
     suppressLoadingSpinner,
   } = restProps;
 
-  const renderStartTimeRef = useRef<number>(0);
+  const renderStartTimeRef = useRef<number>(Logger.getTimestamp());
 
   const shouldRenderChart = useCallback(
     () =>

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,

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.
 }
 

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) => {

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(
     () => (
       <EditorHost
@@ -329,17 +344,25 @@ function Markdown({
 
   const renderPreviewMode = useMemo(
     () => (
-      <SafeMarkdown
-        source={
-          hasError
-            ? MARKDOWN_ERROR_MESSAGE
-            : markdownSource || MARKDOWN_PLACE_HOLDER
-        }
-        htmlSanitization={htmlSanitization}
-        htmlSchemaOverrides={htmlSchemaOverrides}
-      />
+      <ErrorBoundary onError={handleRenderError} showMessage={false}>
+        <SafeMarkdown
+          source={
+            hasError
+              ? MARKDOWN_ERROR_MESSAGE
+              : markdownSource || MARKDOWN_PLACE_HOLDER
+          }
+          htmlSanitization={htmlSanitization}
+          htmlSchemaOverrides={htmlSchemaOverrides}
+        />
+      </ErrorBoundary>
     ),
-    [hasError, markdownSource, htmlSanitization, htmlSchemaOverrides],
+    [
+      hasError,
+      markdownSource,
+      htmlSanitization,
+      htmlSchemaOverrides,
+      handleRenderError,
+    ],
   );
 
   // inherit the size of parent columns

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)

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 (