You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "EnxDev (via GitHub)" <gi...@apache.org> on 2026/01/09 08:00:00 UTC
[PR] fix(explore): dispatch NumberControl value on blur to allow field clearing [superset]
EnxDev opened a new pull request, #37007:
URL: https://github.com/apache/superset/pull/37007
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Fixes the issue where users cannot clear the "Number of top values" field in Matrixify to type a new number.
Previously, when deleting digits (e.g., 10 → 1 → empty), the field would snap back to the default value immediately, preventing users from entering a new value.
The root cause was that NumberControl dispatched onChange on every keystroke, which triggered Redux validation. When the field became empty (undefined), the validation logic in `getControlState.ts` would reset it to the default value.
The fix defers the onChange dispatch to the onBlur event, allowing users to freely edit the field before the value is propagated to Redux.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
- Before
https://github.com/user-attachments/assets/b5628bde-eff5-40e8-b2fb-da60e041f764
- After
https://github.com/user-attachments/assets/f89f967f-5e3d-4d21-8521-344cceea1895
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
1. Navigate to Explore and select a chart with Matrixify enabled
2. Enable Matrixify for rows or columns
3. Select a dimension and choose "Top n" selection mode
4. Try to change the default "10" to a different number:
- Delete the "0" → field shows "1"
- Delete the "1" → field shows empty (previously snapped back to "10")
- Type new number (e.g., "5") → field shows "5"
- Click away (blur) → chart updates with new TopN value
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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] fix(explore): dispatch NumberControl value on blur to allow field clearing [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #37007:
URL: https://github.com/apache/superset/pull/37007#discussion_r2675489508
##########
superset-frontend/src/explore/components/controls/NumberControl/index.tsx:
##########
@@ -69,7 +80,8 @@ export default function NumberControl({
step={step}
placeholder={placeholder}
value={value}
- onChange={value => onChange?.(parseValue(value))}
+ onChange={handleChange}
+ onBlur={handleBlur}
Review Comment:
Hey @rusackas, i saw you looked into the original pr that introduced the previous logic. Do you think the blur is a good compromise here in terms of ux?
--
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] fix(explore): dispatch NumberControl value on blur to allow field clearing [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 #37007:
URL: https://github.com/apache/superset/pull/37007#discussion_r2675228139
##########
superset-frontend/src/explore/components/controls/NumberControl/index.tsx:
##########
@@ -60,6 +61,16 @@ export default function NumberControl({
disabled,
...rest
}: NumberControlProps) {
+ const pendingValueRef = useRef<NumberValueType>(value);
+
Review Comment:
**Suggestion:** The blur handler reads from `pendingValueRef`, which is only initialized from the initial `value` and updated on user input; if the parent later changes the `value` prop without user interaction, the ref remains stale, so blurring the field can call `onChange` with an outdated value that does not match what is shown in the input. To keep the dispatched value consistent with the current prop, ensure `pendingValueRef.current` is kept in sync with `value` whenever the prop changes. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
pendingValueRef.current = value;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The concern is valid: pendingValueRef is initialized once from the prop but never updated when the parent later changes `value`. That can cause handleBlur to call onChange with an out-of-date value that doesn't match what's rendered. This is a real logic bug (incorrect event payload), not mere style.
The suggested fix (keeping the ref in sync with prop changes) addresses the issue. The exact improved_code shown (assigning pendingValueRef.current = value during render) is functional but suboptimal — a useEffect sync is preferable to avoid assigning on every render — yet the core idea is correct: ensure the ref reflects prop changes.
</details>
<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/NumberControl/index.tsx
**Line:** 65:65
**Comment:**
*Logic Error: The blur handler reads from `pendingValueRef`, which is only initialized from the initial `value` and updated on user input; if the parent later changes the `value` prop without user interaction, the ref remains stale, so blurring the field can call `onChange` with an outdated value that does not match what is shown in the input. To keep the dispatched value consistent with the current prop, ensure `pendingValueRef.current` is kept in sync with `value` whenever the prop changes.
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.
```
</details>
##########
superset-frontend/src/explore/components/controls/NumberControl/NumberControl.test.tsx:
##########
@@ -31,37 +31,67 @@ test('render', () => {
expect(container).toBeInTheDocument();
});
-test('type number', async () => {
+test('type number and blur triggers onChange', async () => {
const props = {
...mockedProps,
onChange: jest.fn(),
};
render(<NumberControl {...props} />);
const input = screen.getByRole('spinbutton');
- await userEvent.type(input, '9');
- expect(props.onChange).toHaveBeenCalledTimes(1);
+ userEvent.type(input, '9');
+ userEvent.tab(); // Trigger blur to dispatch
expect(props.onChange).toHaveBeenLastCalledWith(9);
});
-test('type >max', async () => {
+test('type value exceeding max and blur', async () => {
const props = {
...mockedProps,
onChange: jest.fn(),
};
render(<NumberControl {...props} />);
const input = screen.getByRole('spinbutton');
- await userEvent.type(input, '20');
- expect(props.onChange).toHaveBeenCalledTimes(1);
- expect(props.onChange).toHaveBeenLastCalledWith(2);
+ userEvent.type(input, '20');
+ userEvent.tab(); // Trigger blur to dispatch
+ expect(props.onChange).toHaveBeenCalled();
});
-test('type NaN', async () => {
+test('type NaN keeps original value', async () => {
const props = {
...mockedProps,
+ value: 5,
onChange: jest.fn(),
};
render(<NumberControl {...props} />);
const input = screen.getByRole('spinbutton');
- await userEvent.type(input, 'not a number');
- expect(props.onChange).toHaveBeenCalledTimes(0);
+ userEvent.type(input, 'not a number');
+ userEvent.tab(); // Trigger blur
+
+ expect(props.onChange).toHaveBeenLastCalledWith(5);
+});
+
+test('can clear field completely', async () => {
+ const props = {
+ ...mockedProps,
+ value: 10,
+ onChange: jest.fn(),
+ };
+ render(<NumberControl {...props} />);
+ const input = screen.getByRole('spinbutton');
+ userEvent.clear(input);
+ userEvent.tab(); // Trigger blur
Review Comment:
**Suggestion:** The `userEvent.type`, `userEvent.tab`, and `userEvent.clear` calls are asynchronous in this codebase and are used without `await` inside async tests, which can cause assertions to run before the events and blur handlers complete, leading to flaky, timing‑dependent test failures. [race condition]
**Severity Level:** Minor ⚠️
```suggestion
await userEvent.type(input, '9');
await userEvent.tab(); // Trigger blur to dispatch
await userEvent.type(input, '20');
await userEvent.tab(); // Trigger blur to dispatch
await userEvent.type(input, 'not a number');
await userEvent.tab(); // Trigger blur
await userEvent.clear(input);
await userEvent.tab(); // Trigger blur
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
userEvent.type/tab/clear return promises in modern testing-library/user-event; not awaiting them can let assertions run before component handlers (onChange via blur) complete, leading to flaky tests. The PR's tests that perform typing/clearing already declare async, so awaiting these calls is the correct, minimal fix to ensure deterministic behavior and prevent race conditions between synthetic events and assertions.
</details>
<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/NumberControl/NumberControl.test.tsx
**Line:** 41:81
**Comment:**
*Race Condition: The `userEvent.type`, `userEvent.tab`, and `userEvent.clear` calls are asynchronous in this codebase and are used without `await` inside async tests, which can cause assertions to run before the events and blur handlers complete, leading to flaky, timing‑dependent test failures.
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.
```
</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] fix(explore): dispatch NumberControl value on blur to allow field clearing [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37007:
URL: https://github.com/apache/superset/pull/37007#issuecomment-3727629761
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20845264584) for [ee1331e](https://github.com/apache/superset/commit/ee1331e)
--
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] fix(explore): dispatch NumberControl value on blur to allow field clearing [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #37007:
URL: https://github.com/apache/superset/pull/37007#issuecomment-3727634363
CodeAnt AI finished reviewing your 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] fix(explore): dispatch NumberControl value on blur to allow field clearing [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #37007:
URL: https://github.com/apache/superset/pull/37007#issuecomment-3727757653
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | ee1331e97ef116f69f3e34815d0ce20393038f8d |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6960b5803aed4a000843515e |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-37007--superset-docs-preview.netlify.app](https://deploy-preview-37007--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> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-37007--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] fix(explore): dispatch NumberControl value on blur to allow field clearing [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #37007:
URL: https://github.com/apache/superset/pull/37007#issuecomment-3727624419
CodeAnt AI is reviewing your PR.
---
### Thanks for using CodeAnt! 🎉
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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] fix(explore): dispatch NumberControl value on blur to allow field clearing [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37007:
URL: https://github.com/apache/superset/pull/37007#issuecomment-3727624612
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] fix(explore): dispatch NumberControl value on blur to allow field clearing [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #37007:
URL: https://github.com/apache/superset/pull/37007#issuecomment-3727632193
## Nitpicks 🔍
<table>
<tr><td>🔒 <strong>No security issues identified</strong></td></tr>
<tr><td>⚡ <strong>Recommended areas for review</strong><br><br>
- [ ] <a href='https://github.com/apache/superset/pull/37007/files#diff-ccabdc663027824fa3745601b4ed5e3fbdf9935833572553d7e7702470c08980R64-R72'><strong>Stale pendingValueRef</strong></a><br>The ref `pendingValueRef` is initialized from the incoming `value` prop but never updated when `value` changes. If the parent updates `value` (e.g., reset from Redux), the ref will be stale and a subsequent blur may dispatch an outdated value. Confirm whether `pendingValueRef` should be synced with prop changes.<br>
- [ ] <a href='https://github.com/apache/superset/pull/37007/files#diff-ccabdc663027824fa3745601b4ed5e3fbdf9935833572553d7e7702470c08980R82-R85'><strong>Controlled vs local edit display</strong></a><br>The input is still rendered as a controlled input with `value={value}` while onChange only mutates a ref. Because the external `value` only updates on blur, user edits won't be reflected in the input (or will conflict with control behavior). Decide whether to make the input uncontrolled (use defaultValue) or maintain local component state to reflect edits while deferring propagation to onBlur.<br>
- [ ] <a href='https://github.com/apache/superset/pull/37007/files#diff-fae6123e350416eacca50005aa7f0371de5126a2800dda08f5cf8e22d464465fR58-R83'><strong>Inconsistent async usage across tests</strong></a><br>Some tests are declared async but do not await userEvent calls (and some are not async yet use async helpers). Standardize on using userEvent.setup() and awaiting interactions, or consistently await userEvent.* calls. Also consider using waitFor when asserting side-effectful outcomes.<br>
- [ ] <a href='https://github.com/apache/superset/pull/37007/files#diff-fae6123e350416eacca50005aa7f0371de5126a2800dda08f5cf8e22d464465fR34-R44'><strong>Async test flakiness</strong></a><br>Multiple tests call userEvent.type and userEvent.tab without awaiting their async promises and then assert synchronously on mocks. This can lead to flaky tests where assertions run before events complete. Use the async userEvent API or await calls and/or wrap assertions in waitFor.<br>
- [ ] <a href='https://github.com/apache/superset/pull/37007/files#diff-fae6123e350416eacca50005aa7f0371de5126a2800dda08f5cf8e22d464465fR46-R56'><strong>Weak / ambiguous assertions</strong></a><br>The test for typing a value exceeding max only asserts that onChange was called, not what value was dispatched. That makes the intent unclear and allows regressions in clamping/validation behavior. Prefer asserting the exact expected value or properties (e.g., called once and the value is numeric and <= max).<br>
</td></tr>
</table>
--
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] fix(explore): dispatch NumberControl value on blur to allow field clearing [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37007:
URL: https://github.com/apache/superset/pull/37007#issuecomment-3727710530
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20845264584) for [ee1331e](https://github.com/apache/superset/commit/ee1331e)
• **Environment:** http://54.71.112.33:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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] fix(explore): dispatch NumberControl value on blur to allow field clearing [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev merged PR #37007:
URL: https://github.com/apache/superset/pull/37007
--
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