rusackas commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4418056606
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -95,26 +107,65 @@ 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) => {
Review Comment:
@sadpandajoe good catch. The debounced emit now skips any value that no
longer matches the newest keystroke (or was cleared by an external reset), so a
superseded `"1"` is never emitted. Added a test that holds the deferred value
at `"1"` while `"12"` is typed.
##########
superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx:
##########
@@ -118,3 +121,132 @@ 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 before the debounce fires',
() => {
Review Comment:
@sadpandajoe agreed. The blur test now holds `useDeferredValue` at `null`
while typing `abc`, so only the blur handler can deliver it, and asserts a
single `onChange` with none after the debounce.
##########
superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx:
##########
@@ -118,3 +121,132 @@ 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 before the debounce fires',
() => {
+ jest.useFakeTimers();
+ try {
+ const onChange = jest.fn();
+ 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 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' } });
+ rerender(<TextControl {...mockedProps} value="9" onChange={onChange} />);
Review Comment:
@aminghadersohi done. The reset test now holds the deferred value at `12`
across the reset so the queued edit survives until blur.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
