FrancescoCastaldi commented on code in PR #44945:
URL: https://github.com/apache/superset/pull/44945#discussion_r4272837495
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/components/RadioButtonControl.tsx:
##########
@@ -95,7 +104,9 @@ export default function RadioButtonControl({
}`}
onClick={e => {
e.currentTarget?.focus();
- onChange(val);
+ if (isUnpersistedDefault && val === currentValue) {
Review Comment:
Good catch. Re-applying the currently selected shape is indeed an
intentional gesture relied upon by `ZoomConfigControl` and Cartodiagram to
rebuild uniform dimensions or recover drifted local form state.
### Direct Resolution
Ant Design's `Radio.Group.onChange` only fires on value transitions (`val
!== currentValue`) and never emits when clicking an already-selected radio
button. Conversely, `Radio.Button.onClick` previously fired unconditionally,
which caused the duplicate invocation on transitions (#44921).
In commit `8b13f79b`, we partitioned the responsibilities cleanly:
1. `Radio.Button.onClick` now exclusively handles re-clicking the active
option:
```tsx
onClick={e => {
e.currentTarget?.focus();
if (!disabled && val === currentValue) {
onChange(val);
}
}}
```
2. When switching to a different option (`val !== currentValue`), `onClick`
does not call `onChange`, while `Radio.Group.onChange` fires exactly once.
3. When re-clicking the active option (`val === currentValue`),
`Radio.Group.onChange` is inert in DOM/Ant Design, so `onClick` fires
`onChange(val)` exactly once, preserving the rebuild and state-restore gesture.
### Anticipated Edge Case and Preemptive Hardening
We analyzed potential counter-responses and edge cases:
- **Unpersisted and Null Defaults**: We eliminated the fragile
`isUnpersistedDefault` and `hasInitialValueProp` heuristics. Whether `value` is
`undefined`, `null`, or already persisted, clicking the displayed default
option (`val === currentValue`) invokes `onChange` cleanly once.
- **Disabled Option Guard**: We added `!disabled` to the `onClick` condition
to prevent re-clicks on disabled options from erroneously firing `onChange`.
- **Dual TDD Coverage**: Updated `RadioButtonControl.test.tsx` with
dedicated tests asserting that clicking an already-selected option invokes
`onChange` once, verifying unpersisted defaults, and confirming disabled
options remain inert.
--
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]
