Repository navigation
[toggle] Fix grouped cancelation and JSDoc - #4940
Merged
Merged
Conversation
commit: |
Bundle size
PerformanceTotal duration: 1,341.78 ms 🔺+252.77 ms(+23.2%) | Renders: 50 (+0) | Paint: 2,022.77 ms 🔺+389.29 ms(+23.8%)
…and 1 more (+6 within noise) — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
atomiks
marked this pull request as ready for review
May 29, 2026 10:19
atomiks
requested review from
aarongarciah,
colmtuite,
flaviendelangle and
michaldudak
as code owners
May 29, 2026 10:19
flaviendelangle
approved these changes
Jun 1, 2026
flaviendelangle
left a comment
Member
There was a problem hiding this comment.
PR #4940 Review Summary — [toggle] Fix grouped cancelation and JSDoc
The fix itself is correct and well-tested for the headline scenarios. Main gap: the new tests don't cover several adjacent code paths that could regress silently. No blocking issues.
Critical Issues
None.
Important Issues
Test coverage gaps (packages/react/src/toggle/Toggle.test.tsx, packages/react/src/toggle-group/ToggleGroup.test.tsx):
- No test for cancellation in
multiplemode —setGroupValue(ToggleGroup.tsx:70-76) has a distinct array-mutation branch that's exercised only in multi mode. A regression there would slip through. - No test for keyboard activation (Enter/Space) — the existing tests use
button.click(). The repo's convention (seeToggleGroup.test.tsx:402-426) tests both paths separately. - No test asserting
onPressedChangeis called exactly once in the cancel case. A future bug that skips or double-fires it would still pass assertions onaria-pressed.
Suggestions
- Toggle.tsx:90-91 comment wording: "before the group commits" is slightly imprecise —
setGroupValueitself firesonValueChangebefore committing. Consider tightening to: "beforesetGroupValueruns, so canceling here also vetoesonValueChangeand the group value commit (they share thisdetailsobject)." - Toggle.tsx:101-103 (second
isCanceledcheck): still meaningful — guards the case whereonValueChangecancels — but could use a mirroring one-line comment so future readers understand why both checks exist. - Pre-existing silent-failure hazard (not introduced by this PR): if a Toggle inside a ToggleGroup has a falsy
value, the group commit is silently skipped in production (the dev-onlyerror()at Toggle.tsx:52-63 doesn't run in prod). Worth tracking separately.
Strengths
- The reorder genuinely fixes the bug: pre-PR, canceling in
onPressedChangecouldn't veto the group commit becausesetGroupValuehad already run. - The added comment correctly explains the WHY (shared
detailsobject, cancel ordering), not the WHAT — aligned with AGENTS.md guidance. data-disabledis already emitted viastate.disabled(verified byToggle.test.tsx:93); this PR just documents the existing attribute appropriately inToggleDataAttributes.ts,types.md, andpage.mdx.aria-label="Text alignment"on the ToggleGroup hero demos is a real a11y improvement.- Tests follow conventions: Vitest matchers, no Chai chains, no unnecessary
flushMicrotasks, mirror existing describe structure. - No new silent failures, no broad catches, exception propagation is unaffected.
- AGENTS.md compliance: no
window.setTimeout/useCallback/useLayoutEffectviolations.
Recommended Action
- (Optional) Add a
multiplemode cancellation test and a keyboard-activation cancellation test — closest to "important" of any finding here. - (Optional) Tighten the Toggle.tsx:90-91 comment wording and mirror it above the second
isCanceledcheck. - Approve and merge — the fix is correct and reasonably covered.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is part of the Codex review sweep
Changes
onPressedChangecancels the event.data-disabledfor Toggle and add accessible labels to the ToggleGroup hero demos.