sadpandajoe commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4413439906
##########
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:
A queued emit can still fire with an older value after a newer keystroke.
For example, a numeric control queues `"1"`, then the user types `"12"` just
before the 250 ms timer fires while the deferred render for `"12"` has not
committed yet (the case this PR targets: slow Explore renders, or a burst that
keeps interrupting the deferred render). The old timer calls `onChange("1")`,
Explore echoes `1` back, the prop-sync branch resets the input to `1`, and
clears `latestTypedRef`/`pendingValue`, so the `2` is lost with no blur or
external reset involved. The blur fix covers blur only, not this timeout path.
Should the raw `onChangeWrapper` restart the debounce (or the callback skip a
value that differs from `latestTypedRef.current`) so a superseded value is
never emitted?
##########
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:
This blur test would still pass if the
`debouncedEmitRef.current(latestTypedRef.current)` call in `onBlurFlush` were
removed: `fireEvent.change` completes inside its `act`, so the deferred effect
has already queued `'abc'` before blur and `flush()` alone delivers it. Nothing
pins the case the handler exists for, where the deferred value still lags the
raw input. Could this hold the deferred value at `'ab'` (for example by mocking
`useDeferredValue`), change to `'abc'`, blur before it catches up, and assert
one `onChange('abc', [])` immediately and none after advancing past
`FAST_DEBOUNCE`?
--
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]
