rusackas commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4410895490
##########
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:
Good catch. Blur now emits the newest raw keystroke (tracked in a ref), so
the last character survives the deferred-render window; the ref is cleared on
external reset.
##########
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:
Switched to fake timers: type, assert no call, blur, assert exactly one call
with `abc`, then advance past `FAST_DEBOUNCE` and assert still one call.
##########
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:
Added two tests, an external `value` rerender and an unmount, each before
`FAST_DEBOUNCE` elapses, asserting `onChange` is never called and the reset
value stays displayed.
--
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]
