[EnxDev]

SUMMARY

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

• Before

top-n-before.mp4

• After

top-n-after.mp4

TESTING INSTRUCTIONS

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

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • 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

[codeant-ai-for-open-source]

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 ·
Reddit ·
LinkedIn

[bito-code-review]

Code Review Agent Run #115da0

Actionable Suggestions - 0

Additional Suggestions - 3

• superset-frontend/src/explore/components/controls/NumberControl/NumberControl.test.tsx - 1

  • Missing await on userEvent calls · Line 41-41
    userEvent.type and userEvent.tab are not awaited, which may cause tests to be unreliable as the asynchronous events may not be fully processed before assertions.
    Code suggestion

     @@ -41,1 +41,1 @@
    -  userEvent.type(input, '9');
    +  await userEvent.type(input, '9');
     @@ -42,1 +42,1 @@
    -  userEvent.tab(); // Trigger blur to dispatch
    +  await userEvent.tab(); // Trigger blur to dispatch
     @@ -53,1 +53,1 @@
    -  userEvent.type(input, '20');
    +  await userEvent.type(input, '20');
     @@ -54,1 +54,1 @@
    -  userEvent.tab(); // Trigger blur to dispatch
    +  await userEvent.tab(); // Trigger blur to dispatch
     @@ -66,1 +66,1 @@
    -  userEvent.type(input, 'not a number');
    +  await userEvent.type(input, 'not a number');
     @@ -67,1 +67,1 @@
    -  userEvent.tab(); // Trigger blur
    +  await userEvent.tab(); // Trigger blur
     @@ -80,1 +80,1 @@
    -  userEvent.clear(input);
    +  await userEvent.clear(input);
     @@ -81,1 +81,1 @@
    -  userEvent.tab(); // Trigger blur
    +  await userEvent.tab(); // Trigger blur

• superset-frontend/src/explore/components/controls/NumberControl/index.tsx - 2

  • Stale pendingValueRef on prop change · Line 64-64
    pendingValueRef is not updated when the value prop changes, which could lead to stale values being used in onChange calls if the prop updates during user input.
    Code suggestion

     @@ -19,1 +19,1 @@
     import { useRef } from 'react';
    +import { useRef, useEffect } from 'react';
     @@ -64,0 +64,3 @@
    +  useEffect(() => {
    +    pendingValueRef.current = value;
    +  }, [value]);

  • Invalid input resets pendingValueRef · Line 66-66
    When invalid input like NaN is entered, the component sets pendingValueRef to undefined, but the test expects it to keep the original value, which could cause unexpected resets.
    Code suggestion

     @@ -66,2 +69,3 @@
    -  const handleChange = (val: string | number | null) => {
    -    pendingValueRef.current = parseValue(val);
    -  };
    +  const handleChange = (val: string | number | null) => {
    +    const parsed = parseValue(val);
    +    if (parsed !== undefined) pendingValueRef.current = parsed;
    +  };

Review Details

• Files reviewed - 2 · Commit Range: ee1331e..ee1331e

  • superset-frontend/src/explore/components/controls/NumberControl/NumberControl.test.tsx
  • superset-frontend/src/explore/components/controls/NumberControl/index.tsx
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - 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 documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[github-actions]

🎪 Showtime is building environment on GHA for ee1331e

[codeant-ai-for-open-source]

Nitpicks 🔍

🔒 No security issues identified

⚡ Recommended areas for review Stale pendingValueRef 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. Controlled vs local edit display 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. Inconsistent async usage across tests 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. Async test flakiness 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. Weak / ambiguous assertions 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).

[codeant-ai-for-open-source]

CodeAnt AI finished reviewing your PR.

[github-actions]

🎪 Showtime deployed environment on GHA for ee1331e

• Environment: http://54.71.112.33:8080 (admin/admin)
• Lifetime: 48h auto-cleanup
• Updates: New commits create fresh environments automatically

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	ee1331e
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6960b5803aed4a000843515e
😎 Deploy Preview	https://deploy-preview-37007--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[msyavuz]

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?