diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx index 74f65ff95116..a3017158dade 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(); @@ -1733,6 +1725,285 @@ test('does not request datasource metadata when an unrelated control changes', ( } }); +type Axis = 'x' | 'y'; + +// 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 }, + }, + }, +}); + +// 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 }, +]; + +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(); + + // 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(() => { + store.dispatch( + exploreActions.setControlValue(`${axis}_axis_title`, ''), + ); + }); + act(() => { + store.dispatch( + exploreActions.setControlValue( + `${axis}_axis_title`, + '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( + ...programmaticMarginWrite(axis), + ); + expect(marginIn(store, axis)).toBe(75); + } 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'); + + const initialState = axisTitleState(axis, '', 0); + const store = createStore(initialState, reducerIndex); + renderWithRouter({ initialState, store: store as Store }); + setControlValueSpy.mockClear(); + + 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), + ); + }); + + await new Promise(resolve => setTimeout(resolve, 300)); + + 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, + }, + }, + ], + ], + }, + ], + }); + 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(); + + 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(); + } + }, +); + +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(); + } + }, +); test('document.title falls back to the app name on unmount, not the title present at mount', async () => { // A direct page load of an explore URL arrives with the chart name already // in , so restoring the title seen at mount leaves the stale name in diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx index a1507b89c9ca..aff1e900f7bc 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,23 @@ 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); + /** + * 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<ReturnType< + typeof setTimeout + > | null>(null); + const yAxisTitleMarginTimeoutRef = useRef<ReturnType< + typeof setTimeout + > | 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( @@ -793,40 +810,48 @@ function ExploreViewContainer(props: ExploreViewContainerProps) { } } - // Automatically set axis title margins when titles are added or removed - 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, - }); + // 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. + (['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); } - } - - 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, - ); + // 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 (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 (title && currentMargin < 30) { + props.actions.setControlValue(marginKey, 30, undefined, { + programmatic: true, + }); + } 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( @@ -838,6 +863,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. 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..6837f10c998b 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx @@ -16,14 +16,34 @@ * specific language governing permissions and limitations * under the License. */ +import { Constants } from '@superset-ui/core/components'; import { + act, fireEvent, render, screen, userEvent, + waitFor, } 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, @@ -118,3 +138,172 @@ test('should keep showing an externally updated value across later re-renders', rerender(<TextControl {...mockedProps} value="200" label="Row limit" />); expect(screen.getByDisplayValue('200')).toBeInTheDocument(); }); + +test('should not call onChange on mount', () => { + const onChange = jest.fn(); + render(<TextControl {...mockedProps} onChange={onChange} />); + expect(onChange).not.toHaveBeenCalled(); +}); + +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( + <TextControl {...mockedProps} value="1" onChange={onChangeA} />, + ); + 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(<TextControl {...mockedProps} value="1" onChange={onChangeB} />); + 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 () => { + const onChange = jest.fn(); + const { rerender } = render( + <TextControl {...mockedProps} value="75" onChange={onChange} />, + ); + const input = screen.getByRole('textbox'); + await userEvent.clear(input); + await waitFor(() => expect(onChange).toHaveBeenCalledTimes(1)); + rerender(<TextControl {...mockedProps} value="" onChange={onChange} />); + rerender(<TextControl {...mockedProps} value="75" onChange={onChange} />); + await userEvent.clear(input); + 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(<TextControl {...mockedProps} value="" onChange={onChange} />); + await userEvent.type(screen.getByRole('textbox'), '120'); + await waitFor(() => expect(onChange).toHaveBeenCalled()); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith('120', []); +}); + +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(<TextControl {...mockedProps} value="" onChange={onChange} />); + 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 not emit a superseded value when a newer keystroke lands before the timer fires', () => { + jest.useFakeTimers(); + try { + const onChange = jest.fn(); + const { rerender } = render( + <TextControl {...mockedProps} value="" onChange={onChange} />, + ); + 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); + }); + // 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; + rerender(<TextControl {...mockedProps} value="" onChange={onChange} />); + 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 { + const onChange = jest.fn(); + const { rerender } = render( + <TextControl {...mockedProps} value="1" onChange={onChange} />, + ); + fireEvent.change(screen.getByRole('textbox'), { target: { value: '12' } }); + rerender(<TextControl {...mockedProps} value="9" onChange={onChange} />); + act(() => { + jest.advanceTimersByTime(Constants.FAST_DEBOUNCE * 2); + }); + expect(onChange).not.toHaveBeenCalled(); + expect(screen.getByDisplayValue('9')).toBeInTheDocument(); + } finally { + jest.useRealTimers(); + } +}); + +test('should not flush a stale edit on blur after an external reset', () => { + jest.useFakeTimers(); + try { + const onChange = jest.fn(); + const { rerender } = render( + <TextControl {...mockedProps} value="1" onChange={onChange} />, + ); + 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(<TextControl {...mockedProps} value="9" onChange={onChange} />); + 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 { + const onChange = jest.fn(); + const { unmount } = render( + <TextControl {...mockedProps} value="1" onChange={onChange} />, + ); + 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 6f27417fa08b..b224e8bcaaee 100644 --- a/superset-frontend/src/explore/components/controls/TextControl/index.tsx +++ b/superset-frontend/src/explore/components/controls/TextControl/index.tsx @@ -16,10 +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 { debounce } from 'lodash-es'; import { Constants, Input } from '@superset-ui/core/components'; type InputValueType = string | number; @@ -64,6 +71,11 @@ function TextControl<T extends InputValueType = InputValueType>({ }: TextControlProps<T>) { const [localValue, setLocalValue] = useState<string>(safeStringify(value)); const [prevValue, setPrevValue] = useState<T | null | undefined>(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<string | null>(null); + const deferredPendingValue = useDeferredValue(pendingValue); const handleChange = useCallback( (inputValue: string) => { @@ -95,26 +107,74 @@ function TextControl<T extends InputValueType = InputValueType>({ [isFloat, isInt, onChange], ); - const debouncedOnChangeRef = useRef( - debounce((inputValue: string, changeFn: (val: string) => void) => { - changeFn(inputValue); + // 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; + + // 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. + // 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<string | null>(null); + const debouncedEmitRef = useRef( + debounce((inputValue: string) => { + // 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; + handleChangeRef.current(inputValue); }, Constants.FAST_DEBOUNCE), ); + // Fires once the deferred value catches up with the latest keystroke. + // The deferred value keeps the input responsive during a burst of typing; + // the debounce then coalesces what reaches `onChange`. + useEffect(() => { + if (deferredPendingValue !== null) { + debouncedEmitRef.current(deferredPendingValue); + } else { + debouncedEmitRef.current.cancel(); + } + }, [deferredPendingValue]); + useEffect( () => () => { - debouncedOnChangeRef.current.cancel(); + debouncedEmitRef.current.cancel(); }, [], ); + const onBlurFlush = useCallback(() => { + // 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(); + }, []); + const onChangeWrapper = useCallback( (event: ChangeEvent<HTMLInputElement>) => { const { value: newValue } = event.target; + latestTypedRef.current = newValue; setLocalValue(newValue); - debouncedOnChangeRef.current(newValue, handleChange); + setPendingValue(newValue); }, - [handleChange], + [], ); // Sync local value when prop value changes externally. Adjusting state during @@ -125,6 +185,10 @@ function TextControl<T extends InputValueType = InputValueType>({ 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); + latestTypedRef.current = null; } const displayValue = localValue; @@ -144,6 +208,7 @@ function TextControl<T extends InputValueType = InputValueType>({ placeholder={placeholder} onChange={onChangeWrapper} onFocus={onFocus} + onBlur={onBlurFlush} value={displayValue} disabled={disabled} aria-label={ariaLabel ?? label}