You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/10/02 00:56:27 UTC
[PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
rusackas opened a new pull request, #44878:
URL: https://github.com/apache/superset/pull/44878
### SUMMARY
Refs #39890. Continues the `useDeferredValue` adoption started in #39928 (`TableExploreTree`) and #39970 (`DatasourcePanel`), this time for `TextControl`, the plain text input used throughout chart control panels.
Replaces the `lodash` `debounce()` wrapping the `onChange` propagation with `useDeferredValue`. The debounce only ever gated when the parsed value reached the parent via `onChange`, the input's own displayed value (`localValue`) was already updated synchronously on every keystroke, so this is purely a "when does the parent find out" change.
### Why useDeferredValue is strictly better here
- **No fixed timeout.** The deferred value is low-priority work, interrupted by each new keystroke, so a burst of typing never leaves a stale `onChange` call trailing behind it once the timer happens to elapse mid-burst.
- **No manual cleanup.** The old code had to explicitly `.cancel()` the pending debounce on unmount to avoid calling `onChange` after unmount. React never schedules a deferred update for an unmounted component, so that's gone.
### A wrinkle this one has that the first two didn't
`TableExploreTree`/`DatasourcePanel` deferred a value that only ever fed a pure render computation (filtering a list). `TextControl` defers a value that drives an *outbound* `onChange` callback, so two things needed care:
- The branch that mirrors an externally-changed `value` prop back into the displayed text must never itself trigger `onChange`, only an actual keystroke should. Kept a separate `pendingValue` (starts `null`, only ever set by the keystroke handler) so the external-sync branch can't accidentally feed the deferred-value effect.
- `handleChange` is read through a ref inside the effect, so a parent re-render that hands the control a fresh inline `onChange` (common across control-panel controls) doesn't re-fire the effect on its own, pinned by a new test.
### TESTING INSTRUCTIONS
1. Open Explore, find a numeric/text control (e.g. row limit, a named `TextControl`), type into it rapidly.
2. Input should feel responsive throughout; the control panel / chart should update shortly after you stop typing, same end state as before.
3. Programmatically-changed values (undo, reset) should still display correctly.
4. `npm test -- --testPathPatterns='TextControl'` β passes locally (11/11).
### ADDITIONAL INFORMATION
- [x] Has associated issue: #39890
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
π€ Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4325019336
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -799,39 +817,60 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
}
}
- // Automatically set axis title margins when titles are added or removed
+ // 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.
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,
- });
+ if (xAxisTitleMarginTimeoutRef.current) {
+ clearTimeout(xAxisTitleMarginTimeoutRef.current);
}
+ xAxisTitleMarginTimeoutRef.current = setTimeout(() => {
+ const xAxisTitle =
+ latestControlsRef.current.x_axis_title?.value || '';
+ const currentMargin = Number(
+ latestControlsRef.current.x_axis_title_margin?.value ?? 0,
+ );
+
+ if (xAxisTitle && currentMargin < 30) {
Review Comment:
Good catch. The timer now snapshots the margin and viz type when scheduled and bails if either differs when it fires, for both X and Y. Covers a margin pick, a history restore that changes the margin, and a viz switch. Added a test for the margin-pick case.
##########
superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx:
##########
@@ -118,3 +119,40 @@ 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} />);
Review Comment:
Extended the test: it types again after the rerender and asserts onChangeB gets 123 while onChangeA stays at one call.
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -799,39 +817,60 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
}
}
- // Automatically set axis title margins when titles are added or removed
+ // 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.
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,
- });
+ if (xAxisTitleMarginTimeoutRef.current) {
+ clearTimeout(xAxisTitleMarginTimeoutRef.current);
}
+ xAxisTitleMarginTimeoutRef.current = setTimeout(() => {
+ const xAxisTitle =
+ latestControlsRef.current.x_axis_title?.value || '';
+ const currentMargin = Number(
+ latestControlsRef.current.x_axis_title_margin?.value ?? 0,
Review Comment:
Added a test that sets the title, then picks margin 15 inside the 250 ms window, against the real store. It asserts x_axis_title_margin is never set programmatically and stays 15.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4289856588
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -1462,3 +1454,58 @@ test('automatic axis title margin adjustment handles both X and Y axis titles be
jest.restoreAllMocks();
}
});
+
+test('automatic axis title margin adjustment ignores a transient empty value from a quick clear-and-retype', async () => {
+ setupTableChartControlPanel();
+ try {
+ const setControlValueSpy = jest.spyOn(exploreActions, 'setControlValue');
+
+ const initialState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ form_data: {
+ datasource: '1__table',
+ viz_type: VizType.Table,
+ metrics: [],
+ },
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 75 }, // a custom margin above the default 30
+ },
+ },
+ };
+
+ const store = createStore(initialState, reducerIndex);
+ renderWithRouter({ initialState, store: store as Store });
+
+ // Clear any calls from initial render
+ setControlValueSpy.mockClear();
+
+ // TextControl now reports edits via useDeferredValue instead of a fixed
+ // debounce, so a quick clear-and-retype (deleting a title and
+ // immediately typing a replacement) 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.
+ store.dispatch(exploreActions.setControlValue('x_axis_title', ''));
Review Comment:
Good catch, @sadpandajoe. The clear and the retype now each go in their own `act`, so the empty title commits before the retype lands inside the debounce window. I could not run jest in this checkout, so I have not confirmed it goes red against the pre-debounce effect.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #44878:
URL: https://github.com/apache/superset/pull/44878#issuecomment-6193883191
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging because TextControl changes callback timing across all uses of the component.
- Replacing the fixed debounce with useDeferredValue can call onChange for intermediate typed values; verify consumers tolerate the increased callback frequency.
**Assessed commit:** `aae15cfd52d0`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4284321300
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -1462,3 +1454,58 @@ test('automatic axis title margin adjustment handles both X and Y axis titles be
jest.restoreAllMocks();
}
});
+
+test('automatic axis title margin adjustment ignores a transient empty value from a quick clear-and-retype', async () => {
+ setupTableChartControlPanel();
+ try {
+ const setControlValueSpy = jest.spyOn(exploreActions, 'setControlValue');
+
+ const initialState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ form_data: {
+ datasource: '1__table',
+ viz_type: VizType.Table,
+ metrics: [],
+ },
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 75 }, // a custom margin above the default 30
+ },
+ },
+ };
+
+ const store = createStore(initialState, reducerIndex);
+ renderWithRouter({ initialState, store: store as Store });
+
+ // Clear any calls from initial render
+ setControlValueSpy.mockClear();
+
+ // TextControl now reports edits via useDeferredValue instead of a fixed
+ // debounce, so a quick clear-and-retype (deleting a title and
+ // immediately typing a replacement) 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.
+ store.dispatch(exploreActions.setControlValue('x_axis_title', ''));
Review Comment:
This dispatches the clear and the retype back to back in one synchronous block, so React can commit only the final non-empty title and the margin effect never sees the empty value. With the original synchronous margin code the custom margin of 75 would also be left alone here, so this test would pass without the new timeout. Could the clear be committed (its own act) before the retype, with the retype landing inside the debounce window, so the test fails against the pre-debounce effect?
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -95,26 +106,28 @@ function TextControl<T extends InputValueType = InputValueType>({
[isFloat, isInt, onChange],
);
- const debouncedOnChangeRef = useRef(
- debounce((inputValue: string, changeFn: (val: string) => void) => {
- changeFn(inputValue);
- }, Constants.FAST_DEBOUNCE),
- );
+ // 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;
- useEffect(
- () => () => {
- debouncedOnChangeRef.current.cancel();
- },
- [],
- );
+ // Fires once the deferred value catches up with the latest keystroke.
+ // Low-priority and interrupted by each new keystroke, unlike a fixed-delay
+ // debounce, so the input never lags behind a burst of typing.
+ useEffect(() => {
+ if (deferredPendingValue !== null) {
+ handleChangeRef.current(deferredPendingValue);
Review Comment:
Debouncing the axis-margin effect covers one consumer, but TextControl now calls onChange for every keystroke once the deferred render commits, and every renderTrigger control reaches reRenderChart from the same controls effect. For example, typing 120 into Word Cloud's Maximum Font Size (isInt, renderTrigger) now re-lays out the cloud for 1, 12 and 120, where only 120 propagated before the debounce was removed. Could TextControl keep a trailing coalescing step for the outbound onChange, or could you show those other consumers are safe with every intermediate value?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4290343575
##########
superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx:
##########
@@ -118,3 +119,38 @@ 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', () => {
+ const onChangeA = jest.fn();
+ const onChangeB = jest.fn();
+ const { rerender } = render(
+ <TextControl {...mockedProps} onChange={onChangeA} />,
+ );
+
+ // 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} onChange={onChangeB} />);
Review Comment:
No edit happens in this test, so `deferredPendingValue` stays `null` and the effect's null guard suppresses both callbacks regardless of how the effect is wired. An effect that depended on `handleChange` directly (the regression this test is meant to pin) would still pass. Could it type a value, wait for exactly one call to `onChangeA`, then rerender with the same `value` and `onChangeB` and assert `onChangeA` stays at one call and `onChangeB` is never called?
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -799,39 +812,58 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
}
}
- // Automatically set axis title margins when titles are added or removed
+ // 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.
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,
- });
+ if (xAxisTitleMarginTimeoutRef.current) {
+ clearTimeout(xAxisTitleMarginTimeoutRef.current);
}
+ xAxisTitleMarginTimeoutRef.current = setTimeout(() => {
+ const xAxisTitle = props.controls.x_axis_title?.value || '';
+ const currentMargin = Number(
Review Comment:
The timer reads `props.controls` from the render that scheduled it, and a margin-only change doesn't cancel or refresh it. With an empty X-axis title and margin 0, entering a title and then picking margin 75 within the 250 ms window makes the callback see margin 0 and dispatch 30, overwriting the 75 the user just chose (same shape for the Y-axis block). The old synchronous code couldn't clobber a later selection this way. Could the timeout read the latest controls (e.g. through a ref) or be cancelled when the margin control changes?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4343272248
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -844,6 +870,18 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
}
}, [props.controls, props.ownState]);
+ useEffect(
+ () => () => {
Review Comment:
Good catch @sadpandajoe. Added a test for both axes that adds a title, unmounts inside the 250 ms window, advances timers, and asserts no programmatic margin write happens. I could not run jest in this checkout, so CI is its first run.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44878:
URL: https://github.com/apache/superset/pull/44878#issuecomment-5943580782
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | de588c8ca29e3994052ce1e1ddd770a0bc8d495e |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6abf013c8beded00082f247e |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-44878--superset-docs-preview.netlify.app](https://deploy-preview-44878--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">π€</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcvLDYAgEEXRXmYtET9RYweWAcwjLlDIiEZj7F12d3HuS0niljLNRBVZMbtbSyeIrzPurFzcs8SgzgOK4SECVpcJJ4pnpBCfhcsxGOt107nJgsFa66n1bT-Cvh8) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44878--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4283911074
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -95,26 +106,28 @@ function TextControl<T extends InputValueType = InputValueType>({
[isFloat, isInt, onChange],
);
- const debouncedOnChangeRef = useRef(
- debounce((inputValue: string, changeFn: (val: string) => void) => {
- changeFn(inputValue);
- }, Constants.FAST_DEBOUNCE),
- );
+ // 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;
- useEffect(
- () => () => {
- debouncedOnChangeRef.current.cancel();
- },
- [],
- );
+ // Fires once the deferred value catches up with the latest keystroke.
+ // Low-priority and interrupted by each new keystroke, unlike a fixed-delay
+ // debounce, so the input never lags behind a burst of typing.
+ useEffect(() => {
+ if (deferredPendingValue !== null) {
+ handleChangeRef.current(deferredPendingValue);
Review Comment:
Good catch, you're right that useDeferredValue doesn't give the same settled-value guarantee the old debounce did. Rather than reverting TextControl, I debounced the margin effect itself in ExploreViewContainer (same 250ms), so it only applies once the title's actually settled. Added a regression test for the clear-and-retype case. Pushed.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4162707088
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -95,26 +106,28 @@ function TextControl<T extends InputValueType = InputValueType>({
[isFloat, isInt, onChange],
);
- const debouncedOnChangeRef = useRef(
- debounce((inputValue: string, changeFn: (val: string) => void) => {
- changeFn(inputValue);
- }, Constants.FAST_DEBOUNCE),
- );
+ // 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;
- useEffect(
- () => () => {
- debouncedOnChangeRef.current.cancel();
- },
- [],
- );
+ // Fires once the deferred value catches up with the latest keystroke.
+ // Low-priority and interrupted by each new keystroke, unlike a fixed-delay
+ // debounce, so the input never lags behind a burst of typing.
+ useEffect(() => {
+ if (deferredPendingValue !== null) {
+ handleChangeRef.current(deferredPendingValue);
+ }
+ }, [deferredPendingValue]);
const onChangeWrapper = useCallback(
(event: ChangeEvent<HTMLInputElement>) => {
const { value: newValue } = event.target;
setLocalValue(newValue);
- debouncedOnChangeRef.current(newValue, handleChange);
+ setPendingValue(newValue);
Review Comment:
After clearing a field, letting the parent acknowledge it, and undoing the clear, clearing it again updates the input but never calls onChange: pendingValue is still the empty string, so the deferred effect does not run and the chart/save retains the restored value. Could external resets invalidate the pending edit, with a regression test for this edit β acknowledge β reset β identical edit sequence?
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -95,26 +106,28 @@ function TextControl<T extends InputValueType = InputValueType>({
[isFloat, isInt, onChange],
);
- const debouncedOnChangeRef = useRef(
- debounce((inputValue: string, changeFn: (val: string) => void) => {
- changeFn(inputValue);
- }, Constants.FAST_DEBOUNCE),
- );
+ // 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;
- useEffect(
- () => () => {
- debouncedOnChangeRef.current.cancel();
- },
- [],
- );
+ // Fires once the deferred value catches up with the latest keystroke.
+ // Low-priority and interrupted by each new keystroke, unlike a fixed-delay
+ // debounce, so the input never lags behind a burst of typing.
+ useEffect(() => {
+ if (deferredPendingValue !== null) {
+ handleChangeRef.current(deferredPendingValue);
Review Comment:
The deferred render can commit between keystrokes, so clearing an X-axis title and starting its replacement within the old 250 ms debounce window now exposes the empty title to Exploreβs margin-reset effect: a custom margin of 75 becomes 0 and then 30. Could we preserve outbound edit coalescing, or make those downstream effects safe for intermediate input values?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4318403305
##########
superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx:
##########
@@ -118,3 +119,40 @@ 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} />);
Review Comment:
This test only checks that the old and new callbacks are not called after the rerender. If `handleChangeRef.current = handleChange` were removed, every assertion here would still pass, yet the next edit would call the stale `onChangeA` instead of the latest handler. Could it type again after this rerender and assert `onChangeB` receives the new value while `onChangeA` stays at one call?
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -799,39 +817,60 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
}
}
- // Automatically set axis title margins when titles are added or removed
+ // 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.
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,
- });
+ if (xAxisTitleMarginTimeoutRef.current) {
+ clearTimeout(xAxisTitleMarginTimeoutRef.current);
}
+ xAxisTitleMarginTimeoutRef.current = setTimeout(() => {
+ const xAxisTitle =
+ latestControlsRef.current.x_axis_title?.value || '';
+ const currentMargin = Number(
+ latestControlsRef.current.x_axis_title_margin?.value ?? 0,
+ );
+
+ if (xAxisTitle && currentMargin < 30) {
Review Comment:
This timer reads the latest controls when it fires but is only cancelled by another title edit, so any margin change inside the 250 ms window gets overwritten. For example: edit the X title, then pick margin 15 (or press Back to a history entry that restores margin 15 without changing the title) before the timer fires. The callback sees `currentMargin < 30` and dispatches 30 over the user's choice, where the old synchronous code left 15 alone. Switching visualization inside the window has the same shape: the callback runs against the new chart's controls (a Bubble chart has `x_axis_title_margin` but no `x_axis_title`) and resets its margin to 0. Could the pending adjustment be cancelled when the margin changes, a history restore is applied, or the viz type changes? The Y-axis block has the same issue.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4337750330
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -844,6 +870,18 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
}
}, [props.controls, props.ownState]);
+ useEffect(
+ () => () => {
Review Comment:
Nothing exercises this unmount cleanup: none of the new race tests unmounts the container while a margin timer is pending, so deleting both `clearTimeout` calls leaves the suite green. Without it, a title added with margin 0 and then a navigation away inside the 250 ms window would still dispatch the programmatic margin 30 from the departed container. Could a test add a title, unmount before the timer elapses, advance timers, and assert no programmatic `x_axis_title_margin` / `y_axis_title_margin` write happens?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44878:
URL: https://github.com/apache/superset/pull/44878#issuecomment-6196459361
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44878?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `94.59459%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 83.46%. Comparing base ([`f808b98`](https://app.codecov.io/gh/apache/superset/commit/f808b982a59b591359e5fd05bfccfc428bc355b3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`aae15cf`](https://app.codecov.io/gh/apache/superset/commit/aae15cfd52d07c08f1cb6b9f4c08b29bd3f6c333?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44878?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../explore/components/ExploreViewContainer/index.tsx](https://app.codecov.io/gh/apache/superset/pull/44878?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fexplore%2Fcomponents%2FExploreViewContainer%2Findex.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlVmlld0NvbnRhaW5lci9pbmRleC50c3g=) | 92.85% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44878?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44878 +/- ##
=======================================
Coverage 83.45% 83.46%
=======================================
Files 3045 3045
Lines 199180 199196 +16
Branches 46445 46450 +5
=======================================
+ Hits 166234 166250 +16
Misses 29720 29720
Partials 3226 3226
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44878/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/44878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.19% <94.59%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44878?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44878:
URL: https://github.com/apache/superset/pull/44878#issuecomment-5943628017
<!-- Bito Reply -->
The flagged issue is correct. The race condition occurs because `pendingValue` is updated independently of external value resets, causing the deferred `onChange` effect to potentially use a stale `pendingValue` and overwrite the correct reset value. To resolve this, you should ensure that `pendingValue` is synchronized or cleared when the external value changes.
Regarding other comments on this PR, I have checked the available review threads. There are no other actionable suggestions currently listed in the review comments. Would you like me to proceed with implementing the fix for this race condition?
**superset-frontend/src/explore/components/controls/TextControl/index.tsx**
```
useEffect(() => {
setPendingValue(value);
}, [value]);
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44878:
URL: https://github.com/apache/superset/pull/44878#issuecomment-5947286045
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4325367775
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -95,26 +106,28 @@ function TextControl<T extends InputValueType = InputValueType>({
[isFloat, isInt, onChange],
);
- const debouncedOnChangeRef = useRef(
- debounce((inputValue: string, changeFn: (val: string) => void) => {
- changeFn(inputValue);
- }, Constants.FAST_DEBOUNCE),
- );
+ // 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;
- useEffect(
- () => () => {
- debouncedOnChangeRef.current.cancel();
- },
- [],
- );
+ // Fires once the deferred value catches up with the latest keystroke.
+ // Low-priority and interrupted by each new keystroke, unlike a fixed-delay
+ // debounce, so the input never lags behind a burst of typing.
+ useEffect(() => {
+ if (deferredPendingValue !== null) {
+ handleChangeRef.current(deferredPendingValue);
Review Comment:
Because the deferred value can commit between keystrokes, `onChange` now reaches consumers mid-entry, and one of them destroys the input. In the dataset editor's Metrics tab, the Label cell is this control (the `verbose_name` renderer in `DatasourceEditor.tsx`) inside a `CollectionTable` filtered on `metric_name`/`verbose_name`. Search for "Revenue", select the label text and type "Net Revenue": once the deferred "N" commits, `onCellChange` stores it, the row no longer matches the search term, `displayData` drops it, and the focused input unmounts, so typing stops at "N". The old 250ms trailing debounce let a quick replacement finish before the row was filtered.
Could the outbound `onChange` keep a trailing coalescing window (or flush on blur) so downstream filters and `renderTrigger` re-renders only see the settled value?
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -1462,3 +1454,117 @@ test('automatic axis title margin adjustment handles both X and Y axis titles be
jest.restoreAllMocks();
}
});
+
+test('automatic axis title margin adjustment ignores a transient empty value from a quick clear-and-retype', async () => {
+ setupTableChartControlPanel();
+ try {
+ const setControlValueSpy = jest.spyOn(exploreActions, 'setControlValue');
+
+ const initialState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ form_data: {
+ datasource: '1__table',
+ viz_type: VizType.Table,
+ metrics: [],
+ },
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 75 }, // a custom margin above the default 30
+ },
+ },
+ };
+
+ const store = createStore(initialState, reducerIndex);
+ renderWithRouter({ initialState, store: store as Store });
+
+ // Clear any calls from initial render
+ setControlValueSpy.mockClear();
+
+ // TextControl now reports edits via useDeferredValue instead of a fixed
+ // debounce, so a quick clear-and-retype (deleting a title and
+ // immediately typing a replacement) 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('x_axis_title', ''));
+ });
+ act(() => {
+ store.dispatch(
+ exploreActions.setControlValue('x_axis_title', 'X 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(
+ 'x_axis_title_margin',
+ expect.anything(),
+ undefined,
+ { programmatic: true },
+ );
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ jest.restoreAllMocks();
+ }
+});
+
+test('automatic axis title margin adjustment yields to a margin picked inside the debounce window', async () => {
Review Comment:
The two race tests here pin the clear/retype and margin-pick cases, but two guards in the new timer are not exercised. (1) Viz switch / history restore: schedule an adjustment from an empty title and margin 0, switch `viz_type` (or POP a history entry restoring margin 15) before 250ms, advance timers, and assert no programmatic `x_axis_title_margin` write; deleting the `viz_type`/margin snapshot comparison in the timer would still pass every test in this file. (2) Rescheduling: with margin 75, edit the title at 0ms, clear at 200ms, retype at 300ms, each in its own `act`; removing the `clearTimeout` before rescheduling still passes, because both callbacks see the final non-empty title, but with it removed the first timer fires at 250ms on the transient empty value and zeroes the custom margin.
Also, every race test here is X-only, so a Y-only regression to immediate adjustment would pass. Could these be added (parameterized over x/y)?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4291676727
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -799,39 +817,60 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
}
}
- // Automatically set axis title margins when titles are added or removed
+ // 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.
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,
- });
+ if (xAxisTitleMarginTimeoutRef.current) {
+ clearTimeout(xAxisTitleMarginTimeoutRef.current);
}
+ xAxisTitleMarginTimeoutRef.current = setTimeout(() => {
+ const xAxisTitle =
+ latestControlsRef.current.x_axis_title?.value || '';
+ const currentMargin = Number(
+ latestControlsRef.current.x_axis_title_margin?.value ?? 0,
Review Comment:
Nothing pins the switch to `latestControlsRef`: every margin test sets the margin before the timer is scheduled, so reading the captured `props.controls` again would still pass. With an empty X title and margin 0, entering a title and then picking margin 75 inside the 250 ms window would dispatch 30 over the 75. Could a test run that sequence against the real store and assert `x_axis_title_margin` is never set programmatically?
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -95,26 +106,28 @@ function TextControl<T extends InputValueType = InputValueType>({
[isFloat, isInt, onChange],
);
- const debouncedOnChangeRef = useRef(
- debounce((inputValue: string, changeFn: (val: string) => void) => {
- changeFn(inputValue);
- }, Constants.FAST_DEBOUNCE),
- );
+ // 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;
- useEffect(
- () => () => {
- debouncedOnChangeRef.current.cancel();
- },
- [],
- );
+ // Fires once the deferred value catches up with the latest keystroke.
+ // Low-priority and interrupted by each new keystroke, unlike a fixed-delay
+ // debounce, so the input never lags behind a burst of typing.
+ useEffect(() => {
Review Comment:
Without the trailing debounce, every keystroke that gets a deferred commit now reaches `onChange`: typing `120` into Word Cloud's Maximum Font Size (`renderTrigger`) dispatches 1, 12 and 120, and each one runs `reRenderChart` and a fresh layout. It also rewrites the input mid-entry. In an `isFloat` control, typing `1e-12` emits `0.1` at `1e-1`, the echoed prop replaces the text with `0.1`, and the last key produces `0.12` instead of `1e-12`. Could the outbound `onChange` keep a trailing coalescing step, with the deferred value reserved for expensive rendering?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4161952577
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -95,26 +106,28 @@ function TextControl<T extends InputValueType = InputValueType>({
[isFloat, isInt, onChange],
);
- const debouncedOnChangeRef = useRef(
- debounce((inputValue: string, changeFn: (val: string) => void) => {
- changeFn(inputValue);
- }, Constants.FAST_DEBOUNCE),
- );
+ // 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;
- useEffect(
- () => () => {
- debouncedOnChangeRef.current.cancel();
- },
- [],
- );
+ // Fires once the deferred value catches up with the latest keystroke.
+ // Low-priority and interrupted by each new keystroke, unlike a fixed-delay
+ // debounce, so the input never lags behind a burst of typing.
+ useEffect(() => {
+ if (deferredPendingValue !== null) {
+ handleChangeRef.current(deferredPendingValue);
+ }
+ }, [deferredPendingValue]);
const onChangeWrapper = useCallback(
(event: ChangeEvent<HTMLInputElement>) => {
const { value: newValue } = event.target;
setLocalValue(newValue);
- debouncedOnChangeRef.current(newValue, handleChange);
+ setPendingValue(newValue);
Review Comment:
**Suggestion:** After a keystroke, `pendingValue` survives an external value reset, so the deferred effect later sends the stale input through `onChange` and can overwrite the reset.
**Assessment:** π `Major` Β· π `Occurrence: Rarely` Β· π·οΈ `Race condition`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8b92c8ce7fb044fc97e1c99e1413a594&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8b92c8ce7fb044fc97e1c99e1413a594&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/TextControl/index.tsx
**Line:** 128:128
**Comment:**
*Race Condition: After a keystroke, `pendingValue` survives an external value reset, so the deferred effect later sends the stale input through `onChange` and can overwrite the reset.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44878&comment_hash=7968f62ff66951dabd1bf997da43635d069e61ffa06ede3c1fa0c949f3353d8a&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44878&comment_hash=7968f62ff66951dabd1bf997da43635d069e61ffa06ede3c1fa0c949f3353d8a&reaction=dislike'>π</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44878:
URL: https://github.com/apache/superset/pull/44878#issuecomment-5943562986
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4162831263
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -95,26 +106,28 @@ function TextControl<T extends InputValueType = InputValueType>({
[isFloat, isInt, onChange],
);
- const debouncedOnChangeRef = useRef(
- debounce((inputValue: string, changeFn: (val: string) => void) => {
- changeFn(inputValue);
- }, Constants.FAST_DEBOUNCE),
- );
+ // 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;
- useEffect(
- () => () => {
- debouncedOnChangeRef.current.cancel();
- },
- [],
- );
+ // Fires once the deferred value catches up with the latest keystroke.
+ // Low-priority and interrupted by each new keystroke, unlike a fixed-delay
+ // debounce, so the input never lags behind a burst of typing.
+ useEffect(() => {
+ if (deferredPendingValue !== null) {
+ handleChangeRef.current(deferredPendingValue);
+ }
+ }, [deferredPendingValue]);
const onChangeWrapper = useCallback(
(event: ChangeEvent<HTMLInputElement>) => {
const { value: newValue } = event.target;
setLocalValue(newValue);
- debouncedOnChangeRef.current(newValue, handleChange);
+ setPendingValue(newValue);
Review Comment:
Good catch @sadpandajoe. An external value change now clears `pendingValue`, so the repeated identical edit fires `onChange` again. Added a regression test for the edit, acknowledge, reset, identical edit sequence.
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -95,26 +106,28 @@ function TextControl<T extends InputValueType = InputValueType>({
[isFloat, isInt, onChange],
);
- const debouncedOnChangeRef = useRef(
- debounce((inputValue: string, changeFn: (val: string) => void) => {
- changeFn(inputValue);
- }, Constants.FAST_DEBOUNCE),
- );
+ // 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;
- useEffect(
- () => () => {
- debouncedOnChangeRef.current.cancel();
- },
- [],
- );
+ // Fires once the deferred value catches up with the latest keystroke.
+ // Low-priority and interrupted by each new keystroke, unlike a fixed-delay
+ // debounce, so the input never lags behind a burst of typing.
+ useEffect(() => {
+ if (deferredPendingValue !== null) {
+ handleChangeRef.current(deferredPendingValue);
+ }
+ }, [deferredPendingValue]);
const onChangeWrapper = useCallback(
(event: ChangeEvent<HTMLInputElement>) => {
const { value: newValue } = event.target;
setLocalValue(newValue);
- debouncedOnChangeRef.current(newValue, handleChange);
+ setPendingValue(newValue);
Review Comment:
Fixed in 7301bea: the external-value sync now clears `pendingValue`, so a stale edit cannot fire after a reset.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4290956577
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -799,39 +812,58 @@ function ExploreViewContainer(props: ExploreViewContainerProps) {
}
}
- // Automatically set axis title margins when titles are added or removed
+ // 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.
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,
- });
+ if (xAxisTitleMarginTimeoutRef.current) {
+ clearTimeout(xAxisTitleMarginTimeoutRef.current);
}
+ xAxisTitleMarginTimeoutRef.current = setTimeout(() => {
+ const xAxisTitle = props.controls.x_axis_title?.value || '';
+ const currentMargin = Number(
Review Comment:
Good catch. The timeouts now read controls through a ref, so a margin picked during the 250 ms window is what the callback sees and it no longer overwrites it.
##########
superset-frontend/src/explore/components/controls/TextControl/TextControl.test.tsx:
##########
@@ -118,3 +119,38 @@ 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', () => {
+ const onChangeA = jest.fn();
+ const onChangeB = jest.fn();
+ const { rerender } = render(
+ <TextControl {...mockedProps} onChange={onChangeA} />,
+ );
+
+ // 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} onChange={onChangeB} />);
Review Comment:
Agreed, that test could not fail on the regression. It now types a value, waits for one `onChangeA` call, rerenders with a new `onChange`, and asserts `onChangeA` stays at one and `onChangeB` is never called.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(explore): use useDeferredValue for TextControl inputs [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44878:
URL: https://github.com/apache/superset/pull/44878#discussion_r4332217910
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -1462,3 +1454,117 @@ test('automatic axis title margin adjustment handles both X and Y axis titles be
jest.restoreAllMocks();
}
});
+
+test('automatic axis title margin adjustment ignores a transient empty value from a quick clear-and-retype', async () => {
+ setupTableChartControlPanel();
+ try {
+ const setControlValueSpy = jest.spyOn(exploreActions, 'setControlValue');
+
+ const initialState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ form_data: {
+ datasource: '1__table',
+ viz_type: VizType.Table,
+ metrics: [],
+ },
+ controls: {
+ ...reduxState.explore.controls,
+ x_axis_title: { value: 'X Axis Label' },
+ x_axis_title_margin: { value: 75 }, // a custom margin above the default 30
+ },
+ },
+ };
+
+ const store = createStore(initialState, reducerIndex);
+ renderWithRouter({ initialState, store: store as Store });
+
+ // Clear any calls from initial render
+ setControlValueSpy.mockClear();
+
+ // TextControl now reports edits via useDeferredValue instead of a fixed
+ // debounce, so a quick clear-and-retype (deleting a title and
+ // immediately typing a replacement) 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('x_axis_title', ''));
+ });
+ act(() => {
+ store.dispatch(
+ exploreActions.setControlValue('x_axis_title', 'X 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(
+ 'x_axis_title_margin',
+ expect.anything(),
+ undefined,
+ { programmatic: true },
+ );
+ } finally {
+ getChartControlPanelRegistry().remove('table');
+ jest.restoreAllMocks();
+ }
+});
+
+test('automatic axis title margin adjustment yields to a margin picked inside the debounce window', async () => {
Review Comment:
Thanks @sadpandajoe. Added the viz-switch and rescheduling tests, and all the race tests now run for both axes. The viz switch keeps the title and margin so only the viz type snapshot can bail, since a changed margin is already caught by the margin snapshot. Each new test goes red with its guard deleted (the snapshot check, its viz type half alone, and the `clearTimeout`).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org