Skip to content

fix(ToggleSwitch): fire onChange from the interaction instead of an effect#8024

Open
mattcosta7 wants to merge 3 commits into
mainfrom
fix/toggleswitch-onchange-from-handler
Open

fix(ToggleSwitch): fire onChange from the interaction instead of an effect#8024
mattcosta7 wants to merge 3 commits into
mainfrom
fix/toggleswitch-onchange-from-handler

Conversation

@mattcosta7

Copy link
Copy Markdown
Contributor

Overview

For a controlled ToggleSwitch, onChange was fired from an effect rather than from the user interaction:

useEffect(() => {
  if (onChange && isControlled && !disabled) onChange(Boolean(checked))
}, [onChange, checked, isControlled, disabled])

Because the effect is keyed on onChange and checked, this caused onChange to fire:

  • on mount (no user interaction),
  • whenever the parent changes checked (echoing a change the parent already made), and
  • on every render when onChange is an inline function (a new identity each render re-runs the effect).

This PR moves onChange to the interaction handler, where a controlled input should call it, and deletes the effect:

const handleToggleClick = e => {
  if (disabled || loading) return
  const next = !isOn
  if (isControlled) onChange?.(next)   // notify on the actual interaction
  else setIsOn(next)                    // uncontrolled: setIsOn already notifies onChange
  onClick?.(e)
}

Changelog

Fixed

  • ToggleSwitch: onChange no longer fires on mount, on external checked changes, or on every render with an inline handler. It now fires on user interaction, as expected for a controlled input.

Rollout strategy

  • Patch release
  • Minor release
  • Major release
  • None

Testing & Reviewing

  • The existing controlled-onChange test still passes (the click still reports the new value).
  • Added a regression test asserting onChange does not fire on mount or on an external checked change.
  • Full ToggleSwitch suite (17 tests) passes.

Note: this is a behavior change — consumers that (perhaps unintentionally) relied on onChange firing on mount/checked changes will no longer see those calls.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Storybook)
  • Changes are SSR compatible
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

…ffect

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.
@github-actions github-actions Bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Jun 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Action required

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. If this doesn't work, you can also use the original workflow here. Check the integration testing docs for step-by-step instructions. Or, apply the integration-tests: skipped manually label to skip these checks.

To publish a canary release for integration testing, apply the Canary Release label to this PR.

@changeset-bot

changeset-bot Bot commented Jun 23, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: cc87dff

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@primer-integration

primer-integration Bot commented Jun 23, 2026

Copy link
Copy Markdown

Integration test results from github/github-ui PR:

Passed  CI   Passed
Running  VRT   Running
Waiting  Projects   Waiting

@github-actions github-actions Bot temporarily deployed to storybook-preview-8024 June 23, 2026 12:26 Inactive

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes ToggleSwitch controlled-mode behavior so onChange is fired from the actual user interaction instead of a useEffect, preventing unintended calls on mount, on externally-driven checked updates, and on rerenders where onChange changes identity.

Changes:

  • Move controlled onChange invocation into the click handler and remove the effect that previously echoed checked changes.
  • Add a regression test ensuring onChange is not called on mount or when checked changes externally.
  • Add a patch changeset documenting the behavior fix.
Show a summary per file
File Description
packages/react/src/ToggleSwitch/ToggleSwitch.tsx Removes effect-driven onChange and triggers onChange from the interaction handler for controlled usage.
packages/react/src/ToggleSwitch/ToggleSwitch.test.tsx Adds regression coverage for unintended onChange calls in controlled mode.
.changeset/toggleswitch-onchange-from-handler.md Documents the patch-level behavioral fix for consumers.

Copilot's findings

  • Files reviewed: 3/3 changed files
  • Comments generated: 2

Comment on lines +113 to +115
// 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).
Comment on lines +184 to +201
it('does not call onChange on mount or when checked changes externally', () => {
const handleChange = vi.fn()
const {rerender} = render(
<>
<div id="switchLabel">{SWITCH_LABEL_TEXT}</div>
<ToggleSwitch checked={false} onChange={handleChange} aria-labelledby="switchLabel" />
</>,
)
expect(handleChange).not.toHaveBeenCalled()

rerender(
<>
<div id="switchLabel">{SWITCH_LABEL_TEXT}</div>
<ToggleSwitch checked={true} onChange={handleChange} aria-labelledby="switchLabel" />
</>,
)
expect(handleChange).not.toHaveBeenCalled()
})
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants