perf(useMedia): read live matchMedia on first client render via useSyncExternalStore#8001
Conversation
Rewrite four effects flagged by react-hooks/set-state-in-effect to derive state during render or via useSyncExternalStore: - useMedia: use useSyncExternalStore (tear-free, no redundant initial render) - ToggleSwitch: hide loading label via render-time derivation; keep only the delayed reveal in an effect - SelectPanel: reset intermediateSelected by tracking previous open state - ValidationAnimationContainer: mount via render-time derivation Also make the useMedia matchMedia mock reflect real MediaQueryList behavior (matches updates on change).
🦋 Changeset detectedLatest commit: 08cff0d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
github-actions
Bot
added
the
integration-tests: recommended
|
Account for every remaining react-hooks/set-state-in-effect suppression in component/hook source. These are necessary effects (committed-DOM reads or consumer side effects), so add an inline rationale to each rather than a bare disable: - TreeView: getAccessibleName reads committed DOM - LabelGroup: hideChildrenAfterIndex reads DOM children (numeric branch is a possible follow-up derivation) - AutocompleteMenu: sort-on-close shares the effect with onOpenChange - Dialog: commit-timing focus hack - SelectPanel2: reads label textContent from DOM - SelectPanel: no-items announce (commit timing) and first-open fetch (callback) Demo/test suppressions (stories, storybook data) are left for separate cleanup.
|
@copilot resolve the merge conflicts in this pull request |
Resolve conflicts from Add eslint-plugin-react-you-might-not-need-an-effect (#8000): - Keep useSyncExternalStore implementation in useMedia.ts (our refactor) - Keep render-time derivation in ValidationAnimationContainer.tsx (our refactor) - Keep render-time derivation in SelectPanel.tsx intermediateSelected (our refactor) - Keep render-time derivation in ToggleSwitch.tsx; add no-event-handler suppression - Merge descriptive comments with new react-you-might-not-need-an-effect suppressions in Dialog.tsx, AutocompleteMenu.tsx, SelectPanel.tsx, and TreeView.tsx Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
Done — merged
Lint is clean on all resolved files. |
There was a problem hiding this comment.
Pull request overview
This draft PR aims to eliminate suppressions of the react-hooks/set-state-in-effect lint rule by replacing “compute state in an effect” patterns with render-time derivation (or external-store reads), while documenting the remaining intentional effects that must stay as effects (DOM reads, consumer side effects, commit-timing behavior).
Changes:
- Refactors
useMediato useuseSyncExternalStore(SSR-safe snapshot + live client snapshot) and updates its tests to better mirror realMediaQueryListbehavior. - Moves several previously-effect-driven state adjustments to render-time derivation in
ToggleSwitch,SelectPanel, andValidationAnimationContainer. - Adds inline rationale comments for remaining intentional effect-driven state updates (TreeView, LabelGroup, AutocompleteMenu, Dialog, SelectPanel2, SelectPanel).
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/TreeView/TreeView.tsx | Documents why getAccessibleName requires a committed-DOM read in an effect. |
| packages/react/src/ToggleSwitch/ToggleSwitch.tsx | Moves “hide loading label” to render-time derivation; keeps delayed reveal in an effect. |
| packages/react/src/SelectPanel/SelectPanel.tsx | Reworks intermediate single-select modal reset logic to run on open transitions during render; adds effect rationale comments. |
| packages/react/src/LabelGroup/LabelGroup.tsx | Documents why DOM-derived truncation must run in an effect. |
| packages/react/src/internal/components/ValidationAnimationContainer.tsx | Mounts content via render-time derivation; unmount remains animation-driven. |
| packages/react/src/hooks/useMedia.ts | Switches to useSyncExternalStore for tear-free matchMedia reading and SSR snapshot handling. |
| packages/react/src/hooks/tests/useMedia.test.tsx | Improves matchMedia mock so .matches reflects the latest change() updates. |
| packages/react/src/experimental/SelectPanel2/SelectPanel.tsx | Documents committed-DOM requirement for label text reads in an effect. |
| packages/react/src/Dialog/Dialog.tsx | Documents commit-count focus timing hack as intentionally effect-driven. |
| packages/react/src/Autocomplete/AutocompleteMenu.tsx | Documents why close-time sort remains effect-driven due to coupling with onOpenChange. |
| .changeset/derive-state-from-effects.md | Adds patch changeset describing the refactor. |
Copilot's findings
- Files reviewed: 11/11 changed files
- Comments generated: 2
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
mattcosta7
added
Canary Release
mattcosta7
changed the title
|
Rescoped this PR down to just the |
mattcosta7
added
the
integration-tests: skipped manually
|
CI went green in gh/gh-ui, but |
|
Retargets base to main and drops the old #8001 leftovers (useMedia, SelectPanel, ToggleSwitch, ValidationAnimationContainer, Dialog, TreeView, SelectPanel2), which are handled by their own focused PRs. Keeps only the Autocomplete close-time re-sort and LabelGroup numeric-truncation render derivations.
Overview
Status: WIP / draft — not ready to merge. Opening early for direction + discussion.
The
react-hooks/set-state-in-effectlint rule (fromeslint-plugin-react-hooks@7.1.1, the "you might not need an effect" check) flags effects whose only job is to compute state. This PR is a full accounting of every suppression of that rule in the repo: the genuinely-derivable effects are rewritten to compute state during render (or via an external store), and every remaining suppression is either documented in code as an intentional effect or listed below as deferred demo/test cleanup.No public API changes.
Changelog
Changed
useMedianow reads its value viauseSyncExternalStoreinstead ofuseState+useEffect, eliminating the extra render on mount and the SSR/hydrationsetState. On the client, the first render returns the livematchMediavalue rather thandefaultState(which now serves purely as the SSR/hydration snapshot).ToggleSwitchhides the loading status label via render-time derivation; only the delayed reveal remains in an effect (it needs a timer).SelectPanelresets the single-select-modalintermediateSelectedvalue by tracking the previousopenstate during render instead of in an effect.ValidationAnimationContainer(internal) mounts its content via render-time derivation; un-mounting stays deferred toonAnimationEndso the exit animation still plays.useMediatest'smatchMediamock now updatesmatchesonchange(), mirroring realMediaQueryListbehavior (the old mock only fired the event).New
None.
Removed
None.
Rollout strategy
Complete
set-state-in-effectinventoryRepo-wide before this PR: 22 suppressions (11 in component/hook source, 11 in stories + storybook data).
✅ Fixed — genuinely derivable (4)
hooks/useMedia.tsuseSyncExternalStore(tear-free, no redundant initial render)ToggleSwitch.tsxSelectPanel.tsx(intermediateSelected)opentransitioninternal/.../ValidationAnimationContainer.tsxonAnimationEnd🟡 Intentional effects — now documented in code (7)
These trip the rule but are necessary effects (committed-DOM reads or consumer side effects). Each suppression now carries a one-line rationale.
TreeView.tsx(setSubTreeLabel)getAccessibleNameLabelGroup.tsx(hideChildrenAfterIndex)Autocomplete/AutocompleteMenu.tsx(sort-on-close)onOpenChangecallbackonOpenChange(effect)Dialog.tsx(setHasRendered)experimental/SelectPanel2/SelectPanel.tsx(setLabelText)textContentfrom committed DOMSelectPanel.tsx(no-items announce)SelectPanel.tsx(first-open fetch)🟦 Deferred — demo/test code (11, not touched)
Same pattern in stories + storybook data. Lower value and risk of changing story/VRT behavior, so deferred per "clean these separately":
DataTable/storybook/data.ts(1)experimental/SelectPanel2/SelectPanel.examples.stories.tsx(1)FormControl/FormControl.features.stories.tsx(2)SelectPanel/SelectPanel.examples.stories.tsx(4)SelectPanel/SelectPanel.features.stories.tsx(3)Note
Related but not flagged by the rule:
TreeView'ssetIsSubTreeEmptyeffect writes to a parent'sItemContextsetter. The rule only flags same-component state, and this one genuinely can't move to render (cross-component update), so it correctly stays an effect.Testing & Reviewing
All functional changes are behavior-preserving refactors. Validated locally:
useMedia(4),ToggleSwitch(16),FormControl(39),CheckboxGroup(9),RadioGroup(7),SelectPanel(152) unit tests pass.tsc --noEmitandeslintpass on the changed files.Please pay special attention to:
useMedia— the client-first-render value change (live value vsdefaultState). Mirrors whatuseIsMacOSalready does.SelectPanel— the reset now keys onopentransitions (matching the original comment). The previous effect also re-ran onselected/isSingleSelectModalmid-open; not expected for a single-select modal (selection commits on close), but worth confirming.Merge checklist
Important
The 🟦 deferred demo/test items and the 🔜 splittable source effects (
LabelGroupnumeric branch,AutocompleteMenusort-on-close) are intentionally left for separate, individually-tested PRs.