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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
