From 01f22242c010d30ebc2757141d900de88369fcdc Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Mon, 22 Jun 2026 13:05:04 +0000 Subject: [PATCH 1/2] fix(ToggleSwitch): fire onChange from the interaction instead of an effect onChange was called from a useEffect keyed on [onChange, checked, ...], so it fired on mount, whenever a controlled checked value changed externally, and on every render when onChange was an inline function. Call it from the click handler instead. Adds a regression test. --- .../toggleswitch-onchange-from-handler.md | 5 +++++ .../src/ToggleSwitch/ToggleSwitch.test.tsx | 19 +++++++++++++++++++ .../react/src/ToggleSwitch/ToggleSwitch.tsx | 16 ++++++++-------- 3 files changed, 32 insertions(+), 8 deletions(-) create mode 100644 .changeset/toggleswitch-onchange-from-handler.md diff --git a/.changeset/toggleswitch-onchange-from-handler.md b/.changeset/toggleswitch-onchange-from-handler.md new file mode 100644 index 00000000000..c2fc448c00d --- /dev/null +++ b/.changeset/toggleswitch-onchange-from-handler.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +ToggleSwitch: Fire `onChange` from the user interaction instead of an effect, so it no longer fires on mount or when a controlled `checked` value changes externally. diff --git a/packages/react/src/ToggleSwitch/ToggleSwitch.test.tsx b/packages/react/src/ToggleSwitch/ToggleSwitch.test.tsx index bdcbdc45ab1..afc81527896 100644 --- a/packages/react/src/ToggleSwitch/ToggleSwitch.test.tsx +++ b/packages/react/src/ToggleSwitch/ToggleSwitch.test.tsx @@ -181,6 +181,25 @@ describe('ToggleSwitch', () => { expect(handleChange).toHaveBeenCalledWith(true) }) + it('does not call onChange on mount or when checked changes externally', () => { + const handleChange = vi.fn() + const {rerender} = render( + <> +
{SWITCH_LABEL_TEXT}
+ + , + ) + expect(handleChange).not.toHaveBeenCalled() + + rerender( + <> +
{SWITCH_LABEL_TEXT}
+ + , + ) + expect(handleChange).not.toHaveBeenCalled() + }) + it('can pass data attributes to the rendered component', async () => { const TEST_ID = 'a test id' const ControlledSwitchComponent = () => { diff --git a/packages/react/src/ToggleSwitch/ToggleSwitch.tsx b/packages/react/src/ToggleSwitch/ToggleSwitch.tsx index ebe8e7fc512..8d498be9b4b 100644 --- a/packages/react/src/ToggleSwitch/ToggleSwitch.tsx +++ b/packages/react/src/ToggleSwitch/ToggleSwitch.tsx @@ -109,20 +109,20 @@ const ToggleSwitch = React.forwardRef(func e => { if (disabled || loading) return - if (!isControlled) { + if (isControlled) { + // For controlled usage the click is the source of the change, so notify the + // consumer here rather than echoing the `checked` prop back from an effect + // (which fired on mount and whenever `checked` changed). + onChange?.(!isOn) + } else { + // `setIsOn` notifies `onChange` for uncontrolled usage. setIsOn(!isOn) } onClick && onClick(e) }, - [disabled, isControlled, loading, onClick, setIsOn, isOn], + [disabled, isControlled, loading, onClick, onChange, setIsOn, isOn], ) - useEffect(() => { - if (onChange && isControlled && !disabled) { - onChange(Boolean(checked)) - } - }, [onChange, checked, isControlled, disabled]) - useEffect(() => { if (!loading && isLoadingLabelVisible) { // eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-chain-state-updates From cc87dff3339078f192865218870f0c23a1a854df Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Wed, 24 Jun 2026 01:22:34 +0000 Subject: [PATCH 2/2] Clarify effect-dependency comment, add inline onChange regression test --- .../src/ToggleSwitch/ToggleSwitch.test.tsx | 21 +++++++++++++++++++ .../react/src/ToggleSwitch/ToggleSwitch.tsx | 3 ++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/packages/react/src/ToggleSwitch/ToggleSwitch.test.tsx b/packages/react/src/ToggleSwitch/ToggleSwitch.test.tsx index afc81527896..f665ff6d1dc 100644 --- a/packages/react/src/ToggleSwitch/ToggleSwitch.test.tsx +++ b/packages/react/src/ToggleSwitch/ToggleSwitch.test.tsx @@ -200,6 +200,27 @@ describe('ToggleSwitch', () => { expect(handleChange).not.toHaveBeenCalled() }) + it('does not call onChange when a new inline onChange is passed on rerender', () => { + const handleChange = vi.fn() + const {rerender} = render( + <> +
{SWITCH_LABEL_TEXT}
+ handleChange()} aria-labelledby="switchLabel" /> + , + ) + expect(handleChange).not.toHaveBeenCalled() + + // A new `onChange` identity each render previously re-ran the effect and + // fired the callback; it must not be called without a user interaction. + rerender( + <> +
{SWITCH_LABEL_TEXT}
+ handleChange()} aria-labelledby="switchLabel" /> + , + ) + expect(handleChange).not.toHaveBeenCalled() + }) + it('can pass data attributes to the rendered component', async () => { const TEST_ID = 'a test id' const ControlledSwitchComponent = () => { diff --git a/packages/react/src/ToggleSwitch/ToggleSwitch.tsx b/packages/react/src/ToggleSwitch/ToggleSwitch.tsx index 8d498be9b4b..156586378c9 100644 --- a/packages/react/src/ToggleSwitch/ToggleSwitch.tsx +++ b/packages/react/src/ToggleSwitch/ToggleSwitch.tsx @@ -112,7 +112,8 @@ const ToggleSwitch = React.forwardRef(func if (isControlled) { // For controlled usage the click is the source of the change, so notify the // consumer here rather than echoing the `checked` prop back from an effect - // (which fired on mount and whenever `checked` changed). + // (which fired on mount and whenever any of its dependencies — `checked`, + // `disabled`, or `onChange` — changed). onChange?.(!isOn) } else { // `setIsOn` notifies `onChange` for uncontrolled usage.