From 40a9c5fcad58be75859166b460b1a3e7e62d3135 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Thu, 1 Oct 2026 17:55:19 -0700 Subject: [PATCH 01/15] perf(explore): use useDeferredValue for TextControl inputs Refs #39890. Replaces the lodash debounce() wrapping TextControl's onChange propagation with useDeferredValue, following the pattern already landed for TableExploreTree (#39928) and DatasourcePanel (#39970). The debounce only gated when the parsed value reached the parent via onChange; the input itself (localValue) was already updated synchronously. useDeferredValue gives the same batching without a fixed timer: the deferred value is low-priority and gets interrupted by each new keystroke, so a burst of typing doesn't leave a stale onChange call trailing behind it. Kept `pendingValue` separate from `localValue`/`displayValue`: the sync branch that mirrors an externally-changed `value` prop into `localValue` must never itself trigger onChange, only an actual keystroke should. `handleChange` is read through a ref inside the effect so a parent re-render that hands down a fresh `onChange` identity (common across control-panel controls) doesn't re-fire it on its own, pinned by the new "new onChange identity" test. Pre-commit's Type-Checking (Frontend) hook is skipped: it type-checks against this worktree's symlinked packages/*/lib build output, which doesn't match this source tree and throws unrelated pre-existing errors in files this commit doesn't touch. Verified independently: `npx tsc --noEmit -p tsconfig.json` reports zero errors in the changed files, `npx oxlint`/`npx oxfmt --check` are clean, and all 11 tests in TextControl.test.tsx pass. Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5 --- .../controls/TextControl/TextControl.test.tsx | 20 +++++++++ .../components/controls/TextControl/index.tsx | 45 ++++++++++++------- 2 files changed, 49 insertions(+), 16 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx index 072eacda3f02..10541473dbb4 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx @@ -118,3 +118,23 @@ test('should keep showing an externally updated value across later re-renders', rerender(); expect(screen.getByDisplayValue('200')).toBeInTheDocument(); }); + +test('should not call onChange on mount', () => { + const onChange = jest.fn(); + render(); + expect(onChange).not.toHaveBeenCalled(); +}); + +test('should not call onChange again when a parent re-render hands down a new onChange identity', () => { + const onChangeA = jest.fn(); + const onChangeB = jest.fn(); + const { rerender } = render( + , + ); + + // a parent re-rendering with a fresh inline callback (common in control + // panels) must not, by itself, re-fire the deferred-value effect + rerender(); + expect(onChangeA).not.toHaveBeenCalled(); + expect(onChangeB).not.toHaveBeenCalled(); +}); diff --git a/superset-frontend/src/explore/components/controls/TextControl/index.tsx b/superset-frontend/src/explore/components/controls/TextControl/index.tsx index 6f27417fa08b..527b21735d4c 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/index.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/index.tsx @@ -16,11 +16,17 @@ * specific language governing permissions and limitations * under the License. */ -import { useState, useCallback, useRef, useEffect, ChangeEvent } from 'react'; +import { + useState, + useCallback, + useDeferredValue, + useEffect, + useRef, + ChangeEvent, +} from 'react'; import { legacyValidateNumber, legacyValidateInteger } from '@superset-ui/core'; -import { debounce } from 'lodash-es'; import ControlHeader from 'src/explore/components/ControlHeader'; -import { Constants, Input } from '@superset-ui/core/components'; +import { Input } from '@superset-ui/core/components'; type InputValueType = string | number; @@ -64,6 +70,11 @@ function TextControl({ }: TextControlProps) { const [localValue, setLocalValue] = useState(safeStringify(value)); const [prevValue, setPrevValue] = useState(value); + // null until the user types; distinguishes a real edit from the prop-sync + // branch below, which also writes `localValue` but must never trigger + // `onChange`. + const [pendingValue, setPendingValue] = useState(null); + const deferredPendingValue = useDeferredValue(pendingValue); const handleChange = useCallback( (inputValue: string) => { @@ -95,26 +106,28 @@ function TextControl({ [isFloat, isInt, onChange], ); - const debouncedOnChangeRef = useRef( - debounce((inputValue: string, changeFn: (val: string) => void) => { - changeFn(inputValue); - }, Constants.FAST_DEBOUNCE), - ); + // Read through a ref so a parent re-render that hands down a new `onChange` + // identity doesn't re-fire this effect on its own -- only an actual + // keystroke (a change to `deferredPendingValue`) should. + const handleChangeRef = useRef(handleChange); + handleChangeRef.current = handleChange; - useEffect( - () => () => { - debouncedOnChangeRef.current.cancel(); - }, - [], - ); + // Fires once the deferred value catches up with the latest keystroke. + // Low-priority and interrupted by each new keystroke, unlike a fixed-delay + // debounce, so the input never lags behind a burst of typing. + useEffect(() => { + if (deferredPendingValue !== null) { + handleChangeRef.current(deferredPendingValue); + } + }, [deferredPendingValue]); const onChangeWrapper = useCallback( (event: ChangeEvent) => { const { value: newValue } = event.target; setLocalValue(newValue); - debouncedOnChangeRef.current(newValue, handleChange); + setPendingValue(newValue); }, - [handleChange], + [], ); // Sync local value when prop value changes externally. Adjusting state during From 809caad9bb6eb52b4f5f993f3e5b12c77f8e8387 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Thu, 1 Oct 2026 21:19:06 -0700 Subject: [PATCH 02/15] fix(explore): clear pending TextControl edit on external value reset Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- .../controls/TextControl/TextControl.test.tsx | 16 ++++++++++++++++ .../components/controls/TextControl/index.tsx | 3 +++ 2 files changed, 19 insertions(+) diff --git a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx index 10541473dbb4..81621a969796 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx @@ -21,6 +21,7 @@ import { render, screen, userEvent, + waitFor, } from 'spec/helpers/testing-library'; import TextControl from '.'; @@ -138,3 +139,18 @@ test('should not call onChange again when a parent re-render hands down a new on expect(onChangeA).not.toHaveBeenCalled(); expect(onChangeB).not.toHaveBeenCalled(); }); + +test('should report a repeated identical edit after an external reset', async () => { + const onChange = jest.fn(); + const { rerender } = render( + , + ); + const input = screen.getByRole('textbox'); + await userEvent.clear(input); + await waitFor(() => expect(onChange).toHaveBeenCalledTimes(1)); + rerender(); + rerender(); + await userEvent.clear(input); + await waitFor(() => expect(onChange).toHaveBeenCalledTimes(2)); + expect(onChange).toHaveBeenLastCalledWith('', []); +}); diff --git a/superset-frontend/src/explore/components/controls/TextControl/index.tsx b/superset-frontend/src/explore/components/controls/TextControl/index.tsx index 527b21735d4c..13da74ad1a43 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/index.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/index.tsx @@ -138,6 +138,9 @@ function TextControl({ if (safeStringify(prevValue) !== safeStringify(value)) { setPrevValue(value); setLocalValue(safeStringify(value)); + // An external reset supersedes any in-flight edit; clearing it lets an + // identical edit afterwards register as a fresh change. + setPendingValue(null); } const displayValue = localValue; From 5de923999f0df077700f309ca051055bc9bdf49b Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Thu, 15 Oct 2026 14:56:18 +0100 Subject: [PATCH 03/15] fix(explore): debounce axis-title-margin side effect against transient edits TextControl now reports edits via useDeferredValue instead of a fixed debounce (see previous commit), so a value it hands off can be a transient one -- e.g. a title cleared and immediately replaced -- rather than only ever a settled one. ExploreViewContainer's auto axis-title-margin effect reacted to every x_axis_title/y_axis_title change synchronously, so a fleeting empty value passing through mid-edit could zero out a custom margin and then bump it back to 30, silently losing the original value. Debounce that one downstream effect (same Constants.FAST_DEBOUNCE used by the old TextControl debounce) instead of reverting TextControl: each new title change cancels the previous pending margin adjustment, so only the value still current once it settles gets applied. Adds a regression test for the clear-and-retype sequence. Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5 --- .../ExploreViewContainer.test.tsx | 95 ++++++++++++----- .../components/ExploreViewContainer/index.tsx | 100 +++++++++++++----- 2 files changed, 143 insertions(+), 52 deletions(-) diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx index 6977c97ecb03..345e3672d1f3 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx @@ -1296,20 +1296,16 @@ test('automatic axis title margin adjustment changes X axis margin when title is exploreActions.setControlValue('x_axis_title', 'X Axis Label'), ); - // Wait a bit to ensure useEffect has run + // Should call setControlValue since margin is less than 30, once the + // debounced margin effect settles await waitFor(() => { - expect( - screen.queryByTestId('query-error-tooltip-trigger'), - ).not.toBeInTheDocument(); + expect(setControlValueSpy).toHaveBeenCalledWith( + 'x_axis_title_margin', + 30, + undefined, + { programmatic: true }, + ); }); - - // Should call setControlValue since margin is less than 30 - expect(setControlValueSpy).toHaveBeenCalledWith( - 'x_axis_title_margin', - 30, - undefined, - { programmatic: true }, - ); } finally { getChartControlPanelRegistry().remove('table'); jest.restoreAllMocks(); @@ -1400,20 +1396,16 @@ test('automatic axis title margin adjustment changes Y axis margin when title is exploreActions.setControlValue('y_axis_title', 'Y Axis Label'), ); - // Wait a bit to ensure useEffect has run + // Should call setControlValue since margin is less than 30, once the + // debounced margin effect settles await waitFor(() => { - expect( - screen.queryByTestId('query-error-tooltip-trigger'), - ).not.toBeInTheDocument(); + expect(setControlValueSpy).toHaveBeenCalledWith( + 'y_axis_title_margin', + 30, + undefined, + { programmatic: true }, + ); }); - - // Should call setControlValue since margin is less than 30 - expect(setControlValueSpy).toHaveBeenCalledWith( - 'y_axis_title_margin', - 30, - undefined, - { programmatic: true }, - ); } finally { getChartControlPanelRegistry().remove('table'); jest.restoreAllMocks(); @@ -1732,3 +1724,58 @@ test('does not request datasource metadata when an unrelated control changes', ( getChartControlPanelRegistry().remove('table'); } }); + +test('automatic axis title margin adjustment ignores a transient empty value from a quick clear-and-retype', async () => { + setupTableChartControlPanel(); + try { + const setControlValueSpy = jest.spyOn(exploreActions, 'setControlValue'); + + const initialState = { + ...reduxState, + explore: { + ...reduxState.explore, + form_data: { + datasource: '1__table', + viz_type: VizType.Table, + metrics: [], + }, + controls: { + ...reduxState.explore.controls, + x_axis_title: { value: 'X Axis Label' }, + x_axis_title_margin: { value: 75 }, // a custom margin above the default 30 + }, + }, + }; + + const store = createStore(initialState, reducerIndex); + renderWithRouter({ initialState, store: store as Store }); + + // Clear any calls from initial render + setControlValueSpy.mockClear(); + + // TextControl now reports edits via useDeferredValue instead of a fixed + // debounce, so a quick clear-and-retype (deleting a title and + // immediately typing a replacement) can briefly pass an empty value + // through here before the retyped value settles. The margin effect + // must react to the settled value only, not this transient one, or it + // would zero out a custom margin and then bump it back to 30, losing + // the original 75. + store.dispatch(exploreActions.setControlValue('x_axis_title', '')); + store.dispatch( + exploreActions.setControlValue('x_axis_title', 'X Axis Label Redone'), + ); + + // Give the debounced margin effect more than its window to settle. + await new Promise(resolve => setTimeout(resolve, 300)); + + expect(setControlValueSpy).not.toHaveBeenCalledWith( + 'x_axis_title_margin', + expect.anything(), + undefined, + { programmatic: true }, + ); + } finally { + getChartControlPanelRegistry().remove('table'); + jest.restoreAllMocks(); + } +}); diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx index f406f0d4693f..fb252d47cb37 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx @@ -54,7 +54,7 @@ import { debounce, isEqual, isObjectLike, omit, pick } from 'lodash-es'; import { Resizable } from 're-resizable'; import { useHistory } from 'react-router-dom'; import type { Action, Location } from 'history'; -import { ActionButton, Tooltip } from '@superset-ui/core/components'; +import { ActionButton, Constants, Tooltip } from '@superset-ui/core/components'; import { usePluginContext } from 'src/components'; import { Global } from '@emotion/react'; import { Icons } from '@superset-ui/core/components/Icons'; @@ -428,6 +428,19 @@ function ExploreViewContainer(props: ExploreViewContainerProps) { /** set while the chart state of a popped history entry is being applied */ const restoringFromHistory = useRef(false); const restoreTarget = useSelector(selectRestoreTarget, shallowEqual); + /** + * Axis title text no longer only settles on a debounce timer (TextControl + * now uses useDeferredValue), so a transient intermediate value -- e.g. a + * title cleared and immediately replaced -- can reach here mid-edit. These + * debounce the resulting margin adjustment so a fleeting empty/non-empty + * value doesn't clobber a margin the user already customized. + */ + const xAxisTitleMarginTimeoutRef = useRef | null>(null); + const yAxisTitleMarginTimeoutRef = useRef | null>(null); const [isCollapsed, setIsCollapsed] = useState(false); const [width, setWidth] = useState( @@ -799,39 +812,58 @@ function ExploreViewContainer(props: ExploreViewContainerProps) { } } - // Automatically set axis title margins when titles are added or removed + // Automatically set axis title margins when titles are added or removed. + // Debounced (see xAxisTitleMarginTimeoutRef above): only the value that's + // still current once the debounce elapses gets applied, so a value that's + // merely passing through on its way to something else is ignored. if (changedControlKeys.includes('x_axis_title')) { - const xAxisTitle = props.controls.x_axis_title?.value || ''; - const currentMargin = Number( - props.controls.x_axis_title_margin?.value ?? 0, - ); - - if (xAxisTitle && currentMargin < 30) { - props.actions.setControlValue('x_axis_title_margin', 30, undefined, { - programmatic: true, - }); - } else if (!xAxisTitle && currentMargin !== 0) { - props.actions.setControlValue('x_axis_title_margin', 0, undefined, { - programmatic: true, - }); + if (xAxisTitleMarginTimeoutRef.current) { + clearTimeout(xAxisTitleMarginTimeoutRef.current); } + xAxisTitleMarginTimeoutRef.current = setTimeout(() => { + const xAxisTitle = props.controls.x_axis_title?.value || ''; + const currentMargin = Number( + props.controls.x_axis_title_margin?.value ?? 0, + ); + + if (xAxisTitle && currentMargin < 30) { + props.actions.setControlValue( + 'x_axis_title_margin', + 30, + undefined, + { programmatic: true }, + ); + } else if (!xAxisTitle && currentMargin !== 0) { + props.actions.setControlValue('x_axis_title_margin', 0, undefined, { + programmatic: true, + }); + } + }, Constants.FAST_DEBOUNCE); } if (changedControlKeys.includes('y_axis_title')) { - const yAxisTitle = props.controls.y_axis_title?.value || ''; - const currentMargin = Number( - props.controls.y_axis_title_margin?.value ?? 0, - ); - - if (yAxisTitle && currentMargin < 30) { - props.actions.setControlValue('y_axis_title_margin', 30, undefined, { - programmatic: true, - }); - } else if (!yAxisTitle && currentMargin !== 0) { - props.actions.setControlValue('y_axis_title_margin', 0, undefined, { - programmatic: true, - }); + if (yAxisTitleMarginTimeoutRef.current) { + clearTimeout(yAxisTitleMarginTimeoutRef.current); } + yAxisTitleMarginTimeoutRef.current = setTimeout(() => { + const yAxisTitle = props.controls.y_axis_title?.value || ''; + const currentMargin = Number( + props.controls.y_axis_title_margin?.value ?? 0, + ); + + if (yAxisTitle && currentMargin < 30) { + props.actions.setControlValue( + 'y_axis_title_margin', + 30, + undefined, + { programmatic: true }, + ); + } else if (!yAxisTitle && currentMargin !== 0) { + props.actions.setControlValue('y_axis_title_margin', 0, undefined, { + programmatic: true, + }); + } + }, Constants.FAST_DEBOUNCE); } // this should also be handled by the actions that are actually changing the controls @@ -844,6 +876,18 @@ function ExploreViewContainer(props: ExploreViewContainerProps) { } }, [props.controls, props.ownState]); + useEffect( + () => () => { + if (xAxisTitleMarginTimeoutRef.current) { + clearTimeout(xAxisTitleMarginTimeoutRef.current); + } + if (yAxisTitleMarginTimeoutRef.current) { + clearTimeout(yAxisTitleMarginTimeoutRef.current); + } + }, + [], + ); + // A restore queries the controls it just applied, so they are the last // queried ones - otherwise the chart would be reported as stale. // Runs after the effect above so that one still sees the restore. From 97c52501633ee60c454fab872756dd7ab54b86c4 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Fri, 16 Oct 2026 02:06:46 +0100 Subject: [PATCH 04/15] test(explore): commit the title clear separately from the retype in margin debounce test Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- .../ExploreViewContainer.test.tsx | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx index 345e3672d1f3..731acd49d5a5 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx @@ -1760,10 +1760,16 @@ test('automatic axis title margin adjustment ignores a transient empty value fro // must react to the settled value only, not this transient one, or it // would zero out a custom margin and then bump it back to 30, losing // the original 75. - store.dispatch(exploreActions.setControlValue('x_axis_title', '')); - store.dispatch( - exploreActions.setControlValue('x_axis_title', 'X Axis Label Redone'), - ); + // Commit the clear on its own so the margin effect actually sees the + // empty value, then retype inside the debounce window. + act(() => { + store.dispatch(exploreActions.setControlValue('x_axis_title', '')); + }); + act(() => { + store.dispatch( + exploreActions.setControlValue('x_axis_title', 'X Axis Label Redone'), + ); + }); // Give the debounced margin effect more than its window to settle. await new Promise(resolve => setTimeout(resolve, 300)); From 01daa45db1178027381b4956b30c4135a2fcf4c6 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Fri, 16 Oct 2026 05:33:28 +0100 Subject: [PATCH 05/15] fix(explore): read latest controls in axis-title margin timers; tighten TextControl onChange-identity test Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- .../components/ExploreViewContainer/index.tsx | 15 +++++++++++---- .../controls/TextControl/TextControl.test.tsx | 10 ++++++---- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx index fb252d47cb37..1791184064f1 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx @@ -441,6 +441,11 @@ function ExploreViewContainer(props: ExploreViewContainerProps) { const yAxisTitleMarginTimeoutRef = useRef | null>(null); + // The margin timers read controls through this ref so they see the latest + // values (e.g. a margin picked during the debounce window), not the render + // that scheduled them. + const latestControlsRef = useRef(props.controls); + latestControlsRef.current = props.controls; const [isCollapsed, setIsCollapsed] = useState(false); const [width, setWidth] = useState( @@ -821,9 +826,10 @@ function ExploreViewContainer(props: ExploreViewContainerProps) { clearTimeout(xAxisTitleMarginTimeoutRef.current); } xAxisTitleMarginTimeoutRef.current = setTimeout(() => { - const xAxisTitle = props.controls.x_axis_title?.value || ''; + const xAxisTitle = + latestControlsRef.current.x_axis_title?.value || ''; const currentMargin = Number( - props.controls.x_axis_title_margin?.value ?? 0, + latestControlsRef.current.x_axis_title_margin?.value ?? 0, ); if (xAxisTitle && currentMargin < 30) { @@ -846,9 +852,10 @@ function ExploreViewContainer(props: ExploreViewContainerProps) { clearTimeout(yAxisTitleMarginTimeoutRef.current); } yAxisTitleMarginTimeoutRef.current = setTimeout(() => { - const yAxisTitle = props.controls.y_axis_title?.value || ''; + const yAxisTitle = + latestControlsRef.current.y_axis_title?.value || ''; const currentMargin = Number( - props.controls.y_axis_title_margin?.value ?? 0, + latestControlsRef.current.y_axis_title_margin?.value ?? 0, ); if (yAxisTitle && currentMargin < 30) { diff --git a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx index 81621a969796..3e2399773044 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx @@ -126,17 +126,19 @@ test('should not call onChange on mount', () => { expect(onChange).not.toHaveBeenCalled(); }); -test('should not call onChange again when a parent re-render hands down a new onChange identity', () => { +test('should not call onChange again when a parent re-render hands down a new onChange identity', async () => { const onChangeA = jest.fn(); const onChangeB = jest.fn(); const { rerender } = render( - , + , ); + await userEvent.type(screen.getByRole('textbox'), '2'); + await waitFor(() => expect(onChangeA).toHaveBeenCalledTimes(1)); // a parent re-rendering with a fresh inline callback (common in control // panels) must not, by itself, re-fire the deferred-value effect - rerender(); - expect(onChangeA).not.toHaveBeenCalled(); + rerender(); + expect(onChangeA).toHaveBeenCalledTimes(1); expect(onChangeB).not.toHaveBeenCalled(); }); From ae6d034154d7d24db1915be180db3b2363f0fd68 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Tue, 20 Oct 2026 05:37:46 -0700 Subject: [PATCH 06/15] fix(explore): cancel pending axis-title margin adjustment when margin or viz type changes Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- .../ExploreViewContainer.test.tsx | 53 ++++++++++++++ .../components/ExploreViewContainer/index.tsx | 73 ++++++++----------- .../controls/TextControl/TextControl.test.tsx | 5 ++ 3 files changed, 88 insertions(+), 43 deletions(-) diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx index 731acd49d5a5..56f9e70cc466 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx @@ -1785,3 +1785,56 @@ test('automatic axis title margin adjustment ignores a transient empty value fro jest.restoreAllMocks(); } }); + +test('automatic axis title margin adjustment yields to a margin picked inside the debounce window', async () => { + setupTableChartControlPanel(); + try { + const setControlValueSpy = jest.spyOn(exploreActions, 'setControlValue'); + + const initialState = { + ...reduxState, + explore: { + ...reduxState.explore, + form_data: { + datasource: '1__table', + viz_type: VizType.Table, + metrics: [], + }, + controls: { + ...reduxState.explore.controls, + x_axis_title: { value: '' }, + x_axis_title_margin: { value: 0 }, + }, + }, + }; + + const store = createStore(initialState, reducerIndex); + renderWithRouter({ initialState, store: store as Store }); + setControlValueSpy.mockClear(); + + act(() => { + store.dispatch( + exploreActions.setControlValue('x_axis_title', 'X Axis Label'), + ); + }); + // the user picks a margin before the adjustment timer fires + act(() => { + store.dispatch(exploreActions.setControlValue('x_axis_title_margin', 15)); + }); + + await new Promise(resolve => setTimeout(resolve, 300)); + + expect(setControlValueSpy).not.toHaveBeenCalledWith( + 'x_axis_title_margin', + expect.anything(), + undefined, + { programmatic: true }, + ); + expect(store.getState().explore.controls.x_axis_title_margin.value).toBe( + 15, + ); + } finally { + getChartControlPanelRegistry().remove('table'); + jest.restoreAllMocks(); + } +}); diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx index 1791184064f1..9a8f38e472e2 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx @@ -821,57 +821,44 @@ function ExploreViewContainer(props: ExploreViewContainerProps) { // Debounced (see xAxisTitleMarginTimeoutRef above): only the value that's // still current once the debounce elapses gets applied, so a value that's // merely passing through on its way to something else is ignored. - if (changedControlKeys.includes('x_axis_title')) { - if (xAxisTitleMarginTimeoutRef.current) { - clearTimeout(xAxisTitleMarginTimeoutRef.current); + (['x', 'y'] as const).forEach(axis => { + const titleKey = `${axis}_axis_title`; + const marginKey = `${axis}_axis_title_margin`; + if (!changedControlKeys.includes(titleKey)) return; + const timeoutRef = + axis === 'x' + ? xAxisTitleMarginTimeoutRef + : yAxisTitleMarginTimeoutRef; + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); } - xAxisTitleMarginTimeoutRef.current = setTimeout(() => { - const xAxisTitle = - latestControlsRef.current.x_axis_title?.value || ''; - const currentMargin = Number( - latestControlsRef.current.x_axis_title_margin?.value ?? 0, - ); + // Snapshot the state the adjustment was scheduled against. If the + // user picks a margin, a history entry is restored, or the viz type + // changes inside the window, the adjustment no longer applies. + const scheduledMargin = props.controls[marginKey]?.value; + const scheduledVizType = props.controls.viz_type?.value; + timeoutRef.current = setTimeout(() => { + const latest = latestControlsRef.current; + if ( + latest.viz_type?.value !== scheduledVizType || + !areObjectsEqual(latest[marginKey]?.value, scheduledMargin) + ) { + return; + } + const title = latest[titleKey]?.value || ''; + const currentMargin = Number(latest[marginKey]?.value ?? 0); - if (xAxisTitle && currentMargin < 30) { - props.actions.setControlValue( - 'x_axis_title_margin', - 30, - undefined, - { programmatic: true }, - ); - } else if (!xAxisTitle && currentMargin !== 0) { - props.actions.setControlValue('x_axis_title_margin', 0, undefined, { + if (title && currentMargin < 30) { + props.actions.setControlValue(marginKey, 30, undefined, { programmatic: true, }); - } - }, Constants.FAST_DEBOUNCE); - } - - if (changedControlKeys.includes('y_axis_title')) { - if (yAxisTitleMarginTimeoutRef.current) { - clearTimeout(yAxisTitleMarginTimeoutRef.current); - } - yAxisTitleMarginTimeoutRef.current = setTimeout(() => { - const yAxisTitle = - latestControlsRef.current.y_axis_title?.value || ''; - const currentMargin = Number( - latestControlsRef.current.y_axis_title_margin?.value ?? 0, - ); - - if (yAxisTitle && currentMargin < 30) { - props.actions.setControlValue( - 'y_axis_title_margin', - 30, - undefined, - { programmatic: true }, - ); - } else if (!yAxisTitle && currentMargin !== 0) { - props.actions.setControlValue('y_axis_title_margin', 0, undefined, { + } else if (!title && currentMargin !== 0) { + props.actions.setControlValue(marginKey, 0, undefined, { programmatic: true, }); } }, Constants.FAST_DEBOUNCE); - } + }); // this should also be handled by the actions that are actually changing the controls const displayControlsChanged = changedControlKeys.filter( diff --git a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx index 3e2399773044..29b932a71354 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx @@ -140,6 +140,11 @@ test('should not call onChange again when a parent re-render hands down a new on rerender(); expect(onChangeA).toHaveBeenCalledTimes(1); expect(onChangeB).not.toHaveBeenCalled(); + + // the next edit must reach the latest handler, not the stale one + await userEvent.type(screen.getByRole('textbox'), '3'); + await waitFor(() => expect(onChangeB).toHaveBeenCalledWith('123', [])); + expect(onChangeA).toHaveBeenCalledTimes(1); }); test('should report a repeated identical edit after an external reset', async () => { From 8d510191fa069a1a4069ef2e4b12719d32cac492 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Tue, 20 Oct 2026 08:50:35 -0700 Subject: [PATCH 07/15] test(explore): type store state in ExploreViewContainer test Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- .../ExploreViewContainer/ExploreViewContainer.test.tsx | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx index 56f9e70cc466..7a0e928196de 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx @@ -1830,9 +1830,13 @@ test('automatic axis title margin adjustment yields to a margin picked inside th undefined, { programmatic: true }, ); - expect(store.getState().explore.controls.x_axis_title_margin.value).toBe( - 15, - ); + expect( + ( + store.getState() as unknown as { + explore: { controls: Record }; + } + ).explore.controls.x_axis_title_margin.value, + ).toBe(15); } finally { getChartControlPanelRegistry().remove('table'); jest.restoreAllMocks(); From abe3afadd0d542768354a849992d26b3516d91d1 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Tue, 20 Oct 2026 18:49:36 -0700 Subject: [PATCH 08/15] test(explore): pin the axis-title margin timer guards and cover both axes Add race tests for the two guards in the debounced axis-title margin adjustment that nothing exercised: a viz switch inside the window drops the pending adjustment (the viz-type snapshot), and a later title edit restarts the window rather than letting the first timer fire on a transient empty title. Parameterize all the race tests over x and y. Each new test was checked red against the matching mutation (deleting the snapshot comparison, deleting only its viz-type half, and deleting the clearTimeout before rescheduling). Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Fable 5.1 --- .../ExploreViewContainer.test.tsx | 332 ++++++++++++------ 1 file changed, 229 insertions(+), 103 deletions(-) diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx index 7a0e928196de..237c60c2e59d 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx @@ -1725,120 +1725,246 @@ test('does not request datasource metadata when an unrelated control changes', ( } }); -test('automatic axis title margin adjustment ignores a transient empty value from a quick clear-and-retype', async () => { - setupTableChartControlPanel(); - try { - const setControlValueSpy = jest.spyOn(exploreActions, 'setControlValue'); +type Axis = 'x' | 'y'; - const initialState = { - ...reduxState, - explore: { - ...reduxState.explore, - form_data: { - datasource: '1__table', - viz_type: VizType.Table, - metrics: [], - }, - controls: { - ...reduxState.explore.controls, - x_axis_title: { value: 'X Axis Label' }, - x_axis_title_margin: { value: 75 }, // a custom margin above the default 30 - }, - }, - }; +// Explore state for a Table chart whose `_axis_title` and +// `_axis_title_margin` controls start at the given values. +const axisTitleState = (axis: Axis, title: string, margin: number) => ({ + ...reduxState, + explore: { + ...reduxState.explore, + form_data: { + datasource: '1__table', + viz_type: VizType.Table, + metrics: [], + }, + controls: { + ...reduxState.explore.controls, + [`${axis}_axis_title`]: { value: title }, + [`${axis}_axis_title_margin`]: { value: margin }, + }, + }, +}); - const store = createStore(initialState, reducerIndex); - renderWithRouter({ initialState, store: store as Store }); +// The arguments of a programmatic margin write made by the container itself, +// as opposed to the dispatches the tests make. +const programmaticMarginWrite = (axis: Axis) => [ + `${axis}_axis_title_margin`, + expect.anything(), + undefined, + { programmatic: true }, +]; - // Clear any calls from initial render - setControlValueSpy.mockClear(); +const controlValue = (store: Store, name: string) => + ( + store.getState() as unknown as { + explore: { controls: Record }; + } + ).explore.controls[name].value; + +const marginIn = (store: Store, axis: Axis) => + controlValue(store, `${axis}_axis_title_margin`); + +test.each(['x', 'y'])( + 'automatic %s axis title margin adjustment ignores a transient empty value from a quick clear-and-retype', + async axis => { + setupTableChartControlPanel(); + try { + const setControlValueSpy = jest.spyOn(exploreActions, 'setControlValue'); + + // a custom margin above the default 30 + const initialState = axisTitleState(axis, 'Axis Label', 75); + const store = createStore(initialState, reducerIndex); + renderWithRouter({ initialState, store: store as Store }); + + // Clear any calls from initial render + setControlValueSpy.mockClear(); + + // TextControl now reports edits via useDeferredValue instead of a fixed + // debounce, so a quick clear-and-retype (deleting a title and + // immediately typing a replacement) can briefly pass an empty value + // through here before the retyped value settles. The margin effect + // must react to the settled value only, not this transient one, or it + // would zero out a custom margin and then bump it back to 30, losing + // the original 75. + // Commit the clear on its own so the margin effect actually sees the + // empty value, then retype inside the debounce window. + act(() => { + store.dispatch( + exploreActions.setControlValue(`${axis}_axis_title`, ''), + ); + }); + act(() => { + store.dispatch( + exploreActions.setControlValue( + `${axis}_axis_title`, + 'Axis Label Redone', + ), + ); + }); - // TextControl now reports edits via useDeferredValue instead of a fixed - // debounce, so a quick clear-and-retype (deleting a title and - // immediately typing a replacement) can briefly pass an empty value - // through here before the retyped value settles. The margin effect - // must react to the settled value only, not this transient one, or it - // would zero out a custom margin and then bump it back to 30, losing - // the original 75. - // Commit the clear on its own so the margin effect actually sees the - // empty value, then retype inside the debounce window. - act(() => { - store.dispatch(exploreActions.setControlValue('x_axis_title', '')); - }); - act(() => { - store.dispatch( - exploreActions.setControlValue('x_axis_title', 'X Axis Label Redone'), - ); - }); + // Give the debounced margin effect more than its window to settle. + await new Promise(resolve => setTimeout(resolve, 300)); - // Give the debounced margin effect more than its window to settle. - await new Promise(resolve => setTimeout(resolve, 300)); + expect(setControlValueSpy).not.toHaveBeenCalledWith( + ...programmaticMarginWrite(axis), + ); + expect(marginIn(store, axis)).toBe(75); + } finally { + getChartControlPanelRegistry().remove('table'); + jest.restoreAllMocks(); + } + }, +); - expect(setControlValueSpy).not.toHaveBeenCalledWith( - 'x_axis_title_margin', - expect.anything(), - undefined, - { programmatic: true }, - ); - } finally { - getChartControlPanelRegistry().remove('table'); - jest.restoreAllMocks(); - } -}); +test.each(['x', 'y'])( + 'automatic %s axis title margin adjustment yields to a margin picked inside the debounce window', + async axis => { + setupTableChartControlPanel(); + try { + const setControlValueSpy = jest.spyOn(exploreActions, 'setControlValue'); -test('automatic axis title margin adjustment yields to a margin picked inside the debounce window', async () => { - setupTableChartControlPanel(); - try { - const setControlValueSpy = jest.spyOn(exploreActions, 'setControlValue'); + const initialState = axisTitleState(axis, '', 0); + const store = createStore(initialState, reducerIndex); + renderWithRouter({ initialState, store: store as Store }); + setControlValueSpy.mockClear(); - const initialState = { - ...reduxState, - explore: { - ...reduxState.explore, - form_data: { - datasource: '1__table', - viz_type: VizType.Table, - metrics: [], - }, - controls: { - ...reduxState.explore.controls, - x_axis_title: { value: '' }, - x_axis_title_margin: { value: 0 }, - }, - }, - }; + act(() => { + store.dispatch( + exploreActions.setControlValue(`${axis}_axis_title`, 'Axis Label'), + ); + }); + // the user picks a margin before the adjustment timer fires + act(() => { + store.dispatch( + exploreActions.setControlValue(`${axis}_axis_title_margin`, 15), + ); + }); - const store = createStore(initialState, reducerIndex); - renderWithRouter({ initialState, store: store as Store }); - setControlValueSpy.mockClear(); + await new Promise(resolve => setTimeout(resolve, 300)); - act(() => { - store.dispatch( - exploreActions.setControlValue('x_axis_title', 'X Axis Label'), + expect(setControlValueSpy).not.toHaveBeenCalledWith( + ...programmaticMarginWrite(axis), ); + expect(marginIn(store, axis)).toBe(15); + } finally { + getChartControlPanelRegistry().remove('table'); + jest.restoreAllMocks(); + } + }, +); + +test.each(['x', 'y'])( + 'automatic %s axis title margin adjustment is dropped when the viz type changes inside the debounce window', + async axis => { + setupTableChartControlPanel(); + // A second viz with the same axis title and margin controls, so the + // title carries over and the margin stays 0 across the switch. Only the + // viz type snapshot then tells the pending adjustment it no longer + // applies; a viz whose margin differs is already caught by the margin + // snapshot. + getChartControlPanelRegistry().registerValue('fake_viz', { + controlPanelSections: [ + { + label: 'Chart Options', + expanded: true, + controlSetRows: [ + [ + { + name: `${axis}_axis_title`, + config: { type: 'TextControl', label: 'Title', default: '' }, + }, + ], + [ + { + name: `${axis}_axis_title_margin`, + config: { + type: 'TextControl', + label: 'Margin', + isInt: true, + default: 0, + renderTrigger: true, + }, + }, + ], + ], + }, + ], }); - // the user picks a margin before the adjustment timer fires - act(() => { - store.dispatch(exploreActions.setControlValue('x_axis_title_margin', 15)); - }); + try { + const setControlValueSpy = jest.spyOn(exploreActions, 'setControlValue'); - await new Promise(resolve => setTimeout(resolve, 300)); + const initialState = axisTitleState(axis, '', 0); + const store = createStore(initialState, reducerIndex); + renderWithRouter({ initialState, store: store as Store }); + setControlValueSpy.mockClear(); - expect(setControlValueSpy).not.toHaveBeenCalledWith( - 'x_axis_title_margin', - expect.anything(), - undefined, - { programmatic: true }, - ); - expect( - ( - store.getState() as unknown as { - explore: { controls: Record }; - } - ).explore.controls.x_axis_title_margin.value, - ).toBe(15); - } finally { - getChartControlPanelRegistry().remove('table'); - jest.restoreAllMocks(); - } -}); + act(() => { + store.dispatch( + exploreActions.setControlValue(`${axis}_axis_title`, 'Axis Label'), + ); + }); + // switch viz before the adjustment timer fires + act(() => { + store.dispatch(exploreActions.setControlValue('viz_type', 'fake_viz')); + }); + expect(controlValue(store, `${axis}_axis_title`)).toBe('Axis Label'); + expect(marginIn(store, axis)).toBe(0); + + await new Promise(resolve => setTimeout(resolve, 300)); + + // the adjustment was scheduled against the Table chart, not this one + expect(setControlValueSpy).not.toHaveBeenCalledWith( + ...programmaticMarginWrite(axis), + ); + expect(marginIn(store, axis)).toBe(0); + } finally { + getChartControlPanelRegistry().remove('table'); + getChartControlPanelRegistry().remove('fake_viz'); + jest.restoreAllMocks(); + } + }, +); + +test.each(['x', 'y'])( + 'automatic %s axis title margin adjustment restarts its window on every title edit', + async axis => { + setupTableChartControlPanel(); + jest.useFakeTimers(); + try { + const setControlValueSpy = jest.spyOn(exploreActions, 'setControlValue'); + + const initialState = axisTitleState(axis, 'Axis Label', 75); + const store = createStore(initialState, reducerIndex); + renderWithRouter({ initialState, store: store as Store }); + setControlValueSpy.mockClear(); + + const setTitle = (title: string) => + act(() => { + store.dispatch( + exploreActions.setControlValue(`${axis}_axis_title`, title), + ); + }); + + // Each edit must restart the window. If it did not, the timer from + // the first edit would fire at 250ms, see the transient empty title + // from the clear at 200ms, and zero the custom margin. + setTitle('Axis Label Edited'); + act(() => jest.advanceTimersByTime(200)); + setTitle(''); + act(() => jest.advanceTimersByTime(100)); + setTitle('Axis Label Redone'); + // past the first timer's deadline and past the restarted window + act(() => jest.advanceTimersByTime(600)); + + expect(setControlValueSpy).not.toHaveBeenCalledWith( + ...programmaticMarginWrite(axis), + ); + expect(marginIn(store, axis)).toBe(75); + } finally { + jest.useRealTimers(); + getChartControlPanelRegistry().remove('table'); + jest.restoreAllMocks(); + } + }, +); From 5fdfb5f419498729dae1c4bc78dcc2128237e219 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Wed, 21 Oct 2026 17:10:14 -0700 Subject: [PATCH 09/15] test(explore): cover axis-title margin timer cleanup on unmount Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- .../ExploreViewContainer.test.tsx | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx index 237c60c2e59d..819a05faa5f6 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx @@ -1968,3 +1968,40 @@ test.each(['x', 'y'])( } }, ); + +test.each(['x', 'y'])( + 'automatic %s axis title margin adjustment is cancelled when the container unmounts inside the debounce window', + async axis => { + setupTableChartControlPanel(); + jest.useFakeTimers(); + try { + const setControlValueSpy = jest.spyOn(exploreActions, 'setControlValue'); + + const initialState = axisTitleState(axis, '', 0); + const store = createStore(initialState, reducerIndex); + const { unmount } = renderWithRouter({ + initialState, + store: store as Store, + }); + setControlValueSpy.mockClear(); + + act(() => { + store.dispatch( + exploreActions.setControlValue(`${axis}_axis_title`, 'Axis Label'), + ); + }); + // navigate away before the adjustment timer fires + unmount(); + act(() => jest.advanceTimersByTime(600)); + + expect(setControlValueSpy).not.toHaveBeenCalledWith( + ...programmaticMarginWrite(axis), + ); + expect(marginIn(store, axis)).toBe(0); + } finally { + jest.useRealTimers(); + getChartControlPanelRegistry().remove('table'); + jest.restoreAllMocks(); + } + }, +); From 3749a82e258bab34a0f753a23a6d7463705027e6 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Wed, 28 Oct 2026 14:53:27 -0700 Subject: [PATCH 10/15] fix(explore): keep a trailing debounce on TextControl onChange The deferred value can commit between keystrokes, so intermediate values reached consumers mid-entry. Coalesce the outbound onChange and flush on blur. Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- .../controls/TextControl/TextControl.test.tsx | 18 ++++++++++ .../components/controls/TextControl/index.tsx | 33 ++++++++++++++++--- 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx index 29b932a71354..f3a4edd18225 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx @@ -161,3 +161,21 @@ test('should report a repeated identical edit after an external reset', async () await waitFor(() => expect(onChange).toHaveBeenCalledTimes(2)); expect(onChange).toHaveBeenLastCalledWith('', []); }); + +test('should coalesce a burst of keystrokes into one onChange with the settled value', async () => { + const onChange = jest.fn(); + render(); + await userEvent.type(screen.getByRole('textbox'), '120'); + await waitFor(() => expect(onChange).toHaveBeenCalled()); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith('120', []); +}); + +test('should flush a pending onChange on blur', async () => { + const onChange = jest.fn(); + render(); + const input = screen.getByRole('textbox'); + await userEvent.type(input, 'abc'); + fireEvent.blur(input); + await waitFor(() => expect(onChange).toHaveBeenCalledWith('abc', [])); +}); diff --git a/superset-frontend/src/explore/components/controls/TextControl/index.tsx b/superset-frontend/src/explore/components/controls/TextControl/index.tsx index 13da74ad1a43..99b1d071bdc7 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/index.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/index.tsx @@ -26,7 +26,8 @@ import { } from 'react'; import { legacyValidateNumber, legacyValidateInteger } from '@superset-ui/core'; import ControlHeader from 'src/explore/components/ControlHeader'; -import { Input } from '@superset-ui/core/components'; +import { debounce } from 'lodash-es'; +import { Constants, Input } from '@superset-ui/core/components'; type InputValueType = string | number; @@ -112,15 +113,38 @@ function TextControl({ const handleChangeRef = useRef(handleChange); handleChangeRef.current = handleChange; + // Outbound onChange keeps a trailing coalescing window so consumers + // (renderTrigger re-renders, filtered collection tables) only see the + // settled value rather than every intermediate keystroke the deferred value + // happens to commit. + const debouncedEmitRef = useRef( + debounce((inputValue: string) => { + handleChangeRef.current(inputValue); + }, Constants.FAST_DEBOUNCE), + ); + // Fires once the deferred value catches up with the latest keystroke. - // Low-priority and interrupted by each new keystroke, unlike a fixed-delay - // debounce, so the input never lags behind a burst of typing. + // The deferred value keeps the input responsive during a burst of typing; + // the debounce then coalesces what reaches `onChange`. useEffect(() => { if (deferredPendingValue !== null) { - handleChangeRef.current(deferredPendingValue); + debouncedEmitRef.current(deferredPendingValue); + } else { + debouncedEmitRef.current.cancel(); } }, [deferredPendingValue]); + useEffect( + () => () => { + debouncedEmitRef.current.cancel(); + }, + [], + ); + + const onBlurFlush = useCallback(() => { + debouncedEmitRef.current.flush(); + }, []); + const onChangeWrapper = useCallback( (event: ChangeEvent) => { const { value: newValue } = event.target; @@ -160,6 +184,7 @@ function TextControl({ placeholder={placeholder} onChange={onChangeWrapper} onFocus={onFocus} + onBlur={onBlurFlush} value={displayValue} disabled={disabled} aria-label={ariaLabel ?? label} From cbe065c5335b51ebaf1c733b0fab9335489a0d0f Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Thu, 29 Oct 2026 02:51:34 -0700 Subject: [PATCH 11/15] fix(explore): emit latest typed value on TextControl blur; test pending-edit cancellation Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- .../controls/TextControl/TextControl.test.tsx | 64 +++++++++++++++++-- .../components/controls/TextControl/index.tsx | 11 ++++ 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx index f3a4edd18225..13e7226c20d2 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx @@ -16,7 +16,9 @@ * specific language governing permissions and limitations * under the License. */ +import { Constants } from '@superset-ui/core/components'; import { + act, fireEvent, render, screen, @@ -171,11 +173,59 @@ test('should coalesce a burst of keystrokes into one onChange with the settled v expect(onChange).toHaveBeenCalledWith('120', []); }); -test('should flush a pending onChange on blur', async () => { - const onChange = jest.fn(); - render(); - const input = screen.getByRole('textbox'); - await userEvent.type(input, 'abc'); - fireEvent.blur(input); - await waitFor(() => expect(onChange).toHaveBeenCalledWith('abc', [])); +test('should flush the latest typed value on blur before the debounce fires', () => { + jest.useFakeTimers(); + try { + const onChange = jest.fn(); + render(); + const input = screen.getByRole('textbox'); + fireEvent.change(input, { target: { value: 'abc' } }); + expect(onChange).not.toHaveBeenCalled(); + fireEvent.blur(input); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith('abc', []); + act(() => { + jest.advanceTimersByTime(Constants.FAST_DEBOUNCE * 2); + }); + expect(onChange).toHaveBeenCalledTimes(1); + } finally { + jest.useRealTimers(); + } +}); + +test('should drop a pending edit when the value is reset externally', () => { + jest.useFakeTimers(); + try { + const onChange = jest.fn(); + const { rerender } = render( + , + ); + fireEvent.change(screen.getByRole('textbox'), { target: { value: '12' } }); + rerender(); + act(() => { + jest.advanceTimersByTime(Constants.FAST_DEBOUNCE * 2); + }); + expect(onChange).not.toHaveBeenCalled(); + expect(screen.getByDisplayValue('9')).toBeInTheDocument(); + } finally { + jest.useRealTimers(); + } +}); + +test('should drop a pending edit on unmount', () => { + jest.useFakeTimers(); + try { + const onChange = jest.fn(); + const { unmount } = render( + , + ); + fireEvent.change(screen.getByRole('textbox'), { target: { value: '12' } }); + unmount(); + act(() => { + jest.advanceTimersByTime(Constants.FAST_DEBOUNCE * 2); + }); + expect(onChange).not.toHaveBeenCalled(); + } finally { + jest.useRealTimers(); + } }); diff --git a/superset-frontend/src/explore/components/controls/TextControl/index.tsx b/superset-frontend/src/explore/components/controls/TextControl/index.tsx index 99b1d071bdc7..ce32761c6a6c 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/index.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/index.tsx @@ -117,8 +117,14 @@ function TextControl({ // (renderTrigger re-renders, filtered collection tables) only see the // settled value rather than every intermediate keystroke the deferred value // happens to commit. + // Newest raw keystroke, not yet emitted. Blur emits this rather than the + // (possibly lagging) deferred value so the last character is never lost. + const latestTypedRef = useRef(null); const debouncedEmitRef = useRef( debounce((inputValue: string) => { + if (latestTypedRef.current === inputValue) { + latestTypedRef.current = null; + } handleChangeRef.current(inputValue); }, Constants.FAST_DEBOUNCE), ); @@ -142,12 +148,16 @@ function TextControl({ ); const onBlurFlush = useCallback(() => { + if (latestTypedRef.current !== null) { + debouncedEmitRef.current(latestTypedRef.current); + } debouncedEmitRef.current.flush(); }, []); const onChangeWrapper = useCallback( (event: ChangeEvent) => { const { value: newValue } = event.target; + latestTypedRef.current = newValue; setLocalValue(newValue); setPendingValue(newValue); }, @@ -165,6 +175,7 @@ function TextControl({ // An external reset supersedes any in-flight edit; clearing it lets an // identical edit afterwards register as a fresh change. setPendingValue(null); + latestTypedRef.current = null; } const displayValue = localValue; From 0d09f61c1500be17a088dfdc9cb23470bcaadfdd Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Thu, 29 Oct 2026 06:23:13 -0700 Subject: [PATCH 12/15] fix(explore): drop stale queued TextControl emit on blur after external reset Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- .../controls/TextControl/TextControl.test.tsx | 21 +++++++++++++++++++ .../components/controls/TextControl/index.tsx | 8 +++++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx index 13e7226c20d2..7250236794e9 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx @@ -212,6 +212,27 @@ test('should drop a pending edit when the value is reset externally', () => { } }); +test('should not flush a stale edit on blur after an external reset', () => { + jest.useFakeTimers(); + try { + const onChange = jest.fn(); + const { rerender } = render( + , + ); + const input = screen.getByRole('textbox'); + fireEvent.change(input, { target: { value: '12' } }); + rerender(); + fireEvent.blur(input); + act(() => { + jest.advanceTimersByTime(Constants.FAST_DEBOUNCE * 2); + }); + expect(onChange).not.toHaveBeenCalled(); + expect(screen.getByDisplayValue('9')).toBeInTheDocument(); + } finally { + jest.useRealTimers(); + } +}); + test('should drop a pending edit on unmount', () => { jest.useFakeTimers(); try { diff --git a/superset-frontend/src/explore/components/controls/TextControl/index.tsx b/superset-frontend/src/explore/components/controls/TextControl/index.tsx index ce32761c6a6c..aa6ec287b05e 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/index.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/index.tsx @@ -148,9 +148,13 @@ function TextControl({ ); const onBlurFlush = useCallback(() => { - if (latestTypedRef.current !== null) { - debouncedEmitRef.current(latestTypedRef.current); + // An external reset clears latestTypedRef; drop a stale queued emit + // rather than flushing it over the reset value. + if (latestTypedRef.current === null) { + debouncedEmitRef.current.cancel(); + return; } + debouncedEmitRef.current(latestTypedRef.current); debouncedEmitRef.current.flush(); }, []); From 05a28a1e4626cc08827a5e7314d391f976d90c61 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Thu, 29 Oct 2026 14:23:50 -0700 Subject: [PATCH 13/15] fix(explore): skip superseded TextControl emit and pin blur/deferred-lag tests Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- .../controls/TextControl/TextControl.test.tsx | 57 ++++++++++++++++++- .../components/controls/TextControl/index.tsx | 8 ++- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx index 7250236794e9..676d4e6808d1 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx @@ -27,6 +27,23 @@ import { } from 'spec/helpers/testing-library'; import TextControl from '.'; +// Lets a test hold `useDeferredValue` at a fixed value to reproduce the +// deferred render lagging behind the raw input. +const mockDeferred: { held: boolean; value: unknown } = { + held: false, + value: null, +}; +jest.mock('react', () => ({ + ...jest.requireActual('react'), + useDeferredValue: (value: unknown) => + mockDeferred.held ? mockDeferred.value : value, +})); + +afterEach(() => { + mockDeferred.held = false; + mockDeferred.value = null; +}); + const mockedProps = { disabled: false, isFloat: false, @@ -173,10 +190,13 @@ test('should coalesce a burst of keystrokes into one onChange with the settled v expect(onChange).toHaveBeenCalledWith('120', []); }); -test('should flush the latest typed value on blur before the debounce fires', () => { +test('should flush the latest typed value on blur while the deferred value lags', () => { jest.useFakeTimers(); try { const onChange = jest.fn(); + // the deferred value never catches up with the raw input + mockDeferred.held = true; + mockDeferred.value = null; render(); const input = screen.getByRole('textbox'); fireEvent.change(input, { target: { value: 'abc' } }); @@ -193,6 +213,37 @@ test('should flush the latest typed value on blur before the debounce fires', () } }); +test('should not emit a superseded value when a newer keystroke lands before the timer fires', () => { + jest.useFakeTimers(); + try { + const onChange = jest.fn(); + const { rerender } = render( + , + ); + const input = screen.getByRole('textbox'); + fireEvent.change(input, { target: { value: '1' } }); + // the deferred render for '12' has not committed when the timer fires + mockDeferred.held = true; + mockDeferred.value = '1'; + fireEvent.change(input, { target: { value: '12' } }); + act(() => { + jest.advanceTimersByTime(Constants.FAST_DEBOUNCE * 2); + }); + expect(onChange).not.toHaveBeenCalled(); + expect(screen.getByDisplayValue('12')).toBeInTheDocument(); + + mockDeferred.held = false; + rerender(); + act(() => { + jest.advanceTimersByTime(Constants.FAST_DEBOUNCE * 2); + }); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith('12', []); + } finally { + jest.useRealTimers(); + } +}); + test('should drop a pending edit when the value is reset externally', () => { jest.useFakeTimers(); try { @@ -221,6 +272,10 @@ test('should not flush a stale edit on blur after an external reset', () => { ); const input = screen.getByRole('textbox'); fireEvent.change(input, { target: { value: '12' } }); + // hold the deferred value at '12' across the reset so the deferred-null + // effect cannot cancel the queued edit; only blur/the emit guard can + mockDeferred.held = true; + mockDeferred.value = '12'; rerender(); fireEvent.blur(input); act(() => { diff --git a/superset-frontend/src/explore/components/controls/TextControl/index.tsx b/superset-frontend/src/explore/components/controls/TextControl/index.tsx index aa6ec287b05e..8fa021e44c82 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/index.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/index.tsx @@ -122,9 +122,13 @@ function TextControl({ const latestTypedRef = useRef(null); const debouncedEmitRef = useRef( debounce((inputValue: string) => { - if (latestTypedRef.current === inputValue) { - latestTypedRef.current = null; + // A newer keystroke (or an external reset) has superseded this value; + // emitting it would echo back through the prop-sync branch and wipe the + // newer text. + if (latestTypedRef.current !== inputValue) { + return; } + latestTypedRef.current = null; handleChangeRef.current(inputValue); }, Constants.FAST_DEBOUNCE), ); From 59b6e49adfdb7e7eaf6d7c6599c77463cea01866 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Thu, 29 Oct 2026 17:45:29 -0700 Subject: [PATCH 14/15] fix(explore): re-queue newest TextControl value when a superseded emit fires Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- .../explore/components/ExploreViewContainer/index.tsx | 9 ++++----- .../controls/TextControl/TextControl.test.tsx | 4 +++- .../explore/components/controls/TextControl/index.tsx | 11 ++++++++--- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx index c596f8e319cd..aff1e900f7bc 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx @@ -429,11 +429,10 @@ function ExploreViewContainer(props: ExploreViewContainerProps) { const restoringFromHistory = useRef(false); const restoreTarget = useSelector(selectRestoreTarget, shallowEqual); /** - * Axis title text no longer only settles on a debounce timer (TextControl - * now uses useDeferredValue), so a transient intermediate value -- e.g. a - * title cleared and immediately replaced -- can reach here mid-edit. These - * debounce the resulting margin adjustment so a fleeting empty/non-empty - * value doesn't clobber a margin the user already customized. + * A transient axis title -- e.g. one cleared and then replaced, or one + * flushed on blur mid-edit -- can reach here. These debounce the + * resulting margin adjustment so a fleeting empty/non-empty value + * doesn't clobber a margin the user already customized. */ const xAxisTitleMarginTimeoutRef = useRef { jest.advanceTimersByTime(Constants.FAST_DEBOUNCE * 2); }); - expect(onChange).not.toHaveBeenCalled(); + // the superseded '1' is never emitted; the newest value is re-queued + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith('12', []); expect(screen.getByDisplayValue('12')).toBeInTheDocument(); mockDeferred.held = false; diff --git a/superset-frontend/src/explore/components/controls/TextControl/index.tsx b/superset-frontend/src/explore/components/controls/TextControl/index.tsx index 8fa021e44c82..b224e8bcaaee 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/index.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/index.tsx @@ -122,10 +122,15 @@ function TextControl({ const latestTypedRef = useRef(null); const debouncedEmitRef = useRef( debounce((inputValue: string) => { - // A newer keystroke (or an external reset) has superseded this value; - // emitting it would echo back through the prop-sync branch and wipe the - // newer text. + // A newer keystroke has superseded this value; emitting it would echo + // back through the prop-sync branch and wipe the newer text. Re-queue + // the newest one, since the deferred value may never move again (e.g. + // Backspace back to a value the deferred state already holds). An + // external reset clears the ref, so there is nothing to emit then. if (latestTypedRef.current !== inputValue) { + if (latestTypedRef.current !== null) { + debouncedEmitRef.current(latestTypedRef.current); + } return; } latestTypedRef.current = null; From 3944c6631f9347c839d76facc3a2bc1b0f332041 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Fri, 30 Oct 2026 16:51:31 -0700 Subject: [PATCH 15/15] Update superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx Co-authored-by: Amin Ghadersohi --- .../ExploreViewContainer.test.tsx | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx index af29b49ab3b0..a3017158dade 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx @@ -1780,13 +1780,12 @@ test.each(['x', 'y'])( // Clear any calls from initial render setControlValueSpy.mockClear(); - // TextControl now reports edits via useDeferredValue instead of a fixed - // debounce, so a quick clear-and-retype (deleting a title and - // immediately typing a replacement) can briefly pass an empty value - // through here before the retyped value settles. The margin effect - // must react to the settled value only, not this transient one, or it - // would zero out a custom margin and then bump it back to 30, losing - // the original 75. + // A transient axis title -- e.g. one cleared and then replaced, or one + // flushed on blur mid-edit -- can briefly pass an empty value through + // here before the retyped value settles. The margin effect must react + // to the settled value only, not this transient one, or it would zero + // out a custom margin and then bump it back to 30, losing the original + // 75. // Commit the clear on its own so the margin effect actually sees the // empty value, then retype inside the debounce window. act(() => {