You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "anshuman9468 (via GitHub)" <gi...@apache.org> on 2026/10/07 20:14:14 UTC
[PR] fix(chart-controls): prevent duplicate radio button changes [superset]
anshuman9468 opened a new pull request, #45064:
URL: https://github.com/apache/superset/pull/45064
### SUMMARY
Removes the per-button `onChange` dispatch from `RadioButtonControl`. `Radio.Group` is now the sole source of value updates, while the button click handler continues to focus the clicked button.
Fixes #44921
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable; this is an event-handling correction.
### TESTING INSTRUCTIONS
- Run `npx jest packages/superset-ui-chart-controls/test/shared-controls/components/RadioButtonControl.test.tsx --runInBand --silent` from `superset-frontend`.
- Verify selecting a new option calls `onChange` once and clicking the selected option does not call it.
### ADDITIONAL INFORMATION
- [x] 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(chart-controls): prevent duplicate radio button changes [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #45064:
URL: https://github.com/apache/superset/pull/45064#discussion_r4222120580
##########
superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/components/RadioButtonControl.test.tsx:
##########
@@ -88,29 +88,24 @@ test('respects initial value prop', () => {
expect(secondButton).toHaveAttribute('aria-selected', 'true');
});
-test('calls onChange when radio button is clicked', () => {
+test('calls onChange once when selecting a different radio button', () => {
const onChange = jest.fn();
setup({ onChange });
const secondOption = screen.getByText('Option 2');
fireEvent.click(secondOption);
expect(onChange).toHaveBeenCalledWith('option2');
- expect(onChange).toHaveBeenCalled();
+ expect(onChange).toHaveBeenCalledTimes(1);
});
-test('handles multiple clicks correctly', () => {
+test('does not call onChange when clicking the selected radio button', () => {
const onChange = jest.fn();
setup({ onChange });
- fireEvent.click(screen.getByText('Option 2'));
- fireEvent.click(screen.getByText('Option 3'));
fireEvent.click(screen.getByText('Option 1'));
- expect(onChange).toHaveBeenCalledWith('option2');
- expect(onChange).toHaveBeenCalledWith('option3');
- expect(onChange).toHaveBeenCalledWith('option1');
- expect(onChange.mock.calls.length).toBeGreaterThanOrEqual(3);
+ expect(onChange).not.toHaveBeenCalled();
});
Review Comment:
This test does not actually guarantee it's clicking an already-selected option. Unless the default selected value is known to be `Option 1` (and asserted), the click may change selection and legitimately call `onChange`, making the test brittle. Make the selected value explicit (e.g., render with `value: 'option1'` or assert the initial `aria-selected` state for Option 1 before clicking), then click Option 1 and assert `onChange` remains uncalled.
--
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(chart-controls): prevent duplicate radio button changes [superset]
Posted by "anshuman9468 (via GitHub)" <gi...@apache.org>.
anshuman9468 commented on PR #45064:
URL: https://github.com/apache/superset/pull/45064#issuecomment-6046275618
CodeAnt’s risk assessment is a false positive. `Radio.Group` retains its `onChange` handler (`onChange={e => onChange(e.target.value)}`), which is the canonical change path for `Radio.Button` selections. This patch removes only the second, per-button dispatch that caused duplicate callbacks.
The updated regression coverage clicks a different option and asserts exactly one callback, then clicks the already-selected option and asserts no callback. The button click handler remains solely to preserve focus behavior.
--
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(chart-controls): prevent duplicate radio button changes [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45064:
URL: https://github.com/apache/superset/pull/45064#issuecomment-6046081743
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging because radio-button clicks may no longer update the selected value.
- The click handler now only focuses the button, while the test still expects clicking another option to call onChange.
**Assessed commit:** `a6176fc5168f`
--
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