sadpandajoe commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4406789763
##########
superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx:
##########
@@ -118,3 +119,63 @@ 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
() => {
Review Comment:
Nothing pins the cancellation of a pending emit: the reset test above only
resets after `onChange` has already fired, so removing the `else { cancel() }`
branch or the unmount cleanup leaves the suite green. A stale edit would then
reach `onChange` after the control was reset or unmounted. Could a test type,
then rerender with a new external `value` (and another that unmounts) before
`FAST_DEBOUNCE` elapses, advance timers inside `act`, and assert `onChange` is
never called and the reset value stays displayed?
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -95,26 +107,51 @@ 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.
+ const debouncedEmitRef = useRef(
+ debounce((inputValue: string) => {
+ 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(() => {
+ debouncedEmitRef.current.flush();
Review Comment:
`flush()` only emits the last value the deferred effect handed to the
debounce, not the latest keystroke. If "ab" is queued, the user types "abc" and
tabs away before the deferred render commits, blur emits "ab"; Explore echoes
that back, the external-sync branch resets the input to "ab" and clears
`pendingValue`, so the "c" is lost for good. The old handler passed the newest
raw value straight to the debounce, so this window is new. Could blur emit the
latest typed value (e.g. keep it in a ref from `onChangeWrapper` and feed it to
the debounce before flushing)?
##########
superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx:
##########
@@ -118,3 +119,63 @@ 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 a pending onChange on blur', async () => {
+ const onChange = jest.fn();
+ render(<TextControl {...mockedProps} value="" onChange={onChange} />);
+ const input = screen.getByRole('textbox');
+ await userEvent.type(input, 'abc');
+ fireEvent.blur(input);
Review Comment:
This test passes with the `onBlur` handler removed: the trailing debounce
fires within `waitFor`'s default timeout and delivers `'abc'` anyway. Could it
use fake timers, type, assert `onChange` has not been called, blur, assert
exactly one call with `'abc'` immediately, then advance past `FAST_DEBOUNCE`
and assert no second call?
--
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]
