From 4e6cf0d1715e0371169b291be7a503f2d15f7bc5 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 16 Jun 2026 14:01:02 +0000 Subject: [PATCH 1/6] perf: derive state during render instead of effects 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/derive-state-from-effects.md | 5 + .../react/src/SelectPanel/SelectPanel.tsx | 11 +- .../react/src/ToggleSwitch/ToggleSwitch.tsx | 12 ++- .../src/hooks/__tests__/useMedia.test.tsx | 6 +- packages/react/src/hooks/useMedia.ts | 101 ++++++++---------- .../ValidationAnimationContainer.tsx | 12 ++- 6 files changed, 79 insertions(+), 68 deletions(-) create mode 100644 .changeset/derive-state-from-effects.md diff --git a/.changeset/derive-state-from-effects.md b/.changeset/derive-state-from-effects.md new file mode 100644 index 00000000000..19c9ae562f4 --- /dev/null +++ b/.changeset/derive-state-from-effects.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Derive state during render instead of in effects for `useMedia`, `ToggleSwitch`, `SelectPanel`, and the internal validation animation container, removing redundant render passes (no public API changes). diff --git a/packages/react/src/SelectPanel/SelectPanel.tsx b/packages/react/src/SelectPanel/SelectPanel.tsx index 95cbe2c6ad8..013a6e3c0e1 100644 --- a/packages/react/src/SelectPanel/SelectPanel.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.tsx @@ -241,11 +241,14 @@ function Panel({ isSingleSelectModal ? selected : undefined, ) - // Reset the intermediate selected item when the panel is open/closed - useEffect(() => { - // eslint-disable-next-line react-hooks/set-state-in-effect + // Reset the intermediate selected item when the panel is opened or closed. + // Tracking the previous `open` value lets us derive this during render instead + // of in an effect, avoiding an extra render pass each time the panel toggles. + const [prevOpen, setPrevOpen] = useState(open) + if (prevOpen !== open) { + setPrevOpen(open) setIntermediateSelected(isSingleSelectModal ? selected : undefined) - }, [isSingleSelectModal, open, selected]) + } const onListContainerRefChanged: FilteredActionListProps['onListContainerRefChanged'] = useCallback( (node: HTMLElement | null) => { diff --git a/packages/react/src/ToggleSwitch/ToggleSwitch.tsx b/packages/react/src/ToggleSwitch/ToggleSwitch.tsx index f36c21dccb4..b06318e3267 100644 --- a/packages/react/src/ToggleSwitch/ToggleSwitch.tsx +++ b/packages/react/src/ToggleSwitch/ToggleSwitch.tsx @@ -123,11 +123,15 @@ const ToggleSwitch = React.forwardRef(func } }, [onChange, checked, isControlled, disabled]) + // Hiding the loading label is pure state derived from `loading`, so compute it + // during render. Only the delayed reveal needs a timer, which stays in the + // effect below. + if (!loading && isLoadingLabelVisible) { + setIsLoadingLabelVisible(false) + } + useEffect(() => { - if (!loading && isLoadingLabelVisible) { - // eslint-disable-next-line react-hooks/set-state-in-effect - setIsLoadingLabelVisible(false) - } else if (loading && !isLoadingLabelVisible) { + if (loading && !isLoadingLabelVisible) { safeSetTimeout(() => { setIsLoadingLabelVisible(true) }, loadingLabelDelay) diff --git a/packages/react/src/hooks/__tests__/useMedia.test.tsx b/packages/react/src/hooks/__tests__/useMedia.test.tsx index 2884c18f2da..2a4684b08c7 100644 --- a/packages/react/src/hooks/__tests__/useMedia.test.tsx +++ b/packages/react/src/hooks/__tests__/useMedia.test.tsx @@ -8,11 +8,14 @@ type MediaQueryEventListener = (event: {matches: boolean}) => void function mockMatchMedia({defaultMatch = false} = {}) { const listeners = new Set() + // Track the current match state so that reading `matchMedia(query).matches` + // reflects the latest value, mirroring real `MediaQueryList` behavior. + let currentMatches = defaultMatch Object.defineProperty(window, 'matchMedia', { writable: true, value: vi.fn().mockImplementation(query => ({ - matches: defaultMatch, + matches: currentMatches, media: query, onchange: null, addListener: vi.fn(), // deprecated @@ -29,6 +32,7 @@ function mockMatchMedia({defaultMatch = false} = {}) { return { change({matches = false}) { + currentMatches = matches for (const listener of listeners) { listener({ matches, diff --git a/packages/react/src/hooks/useMedia.ts b/packages/react/src/hooks/useMedia.ts index d70d9bccfb7..b818b0adb1f 100644 --- a/packages/react/src/hooks/useMedia.ts +++ b/packages/react/src/hooks/useMedia.ts @@ -1,5 +1,4 @@ -import React, {useContext, useEffect} from 'react' -import {canUseDOM} from '../utils/environment' +import {useCallback, useContext, useSyncExternalStore} from 'react' import {warning} from '../utils/warning' import {MatchMediaContext} from './MatchMediaContext' @@ -18,71 +17,65 @@ import {MatchMediaContext} from './MatchMediaContext' */ export function useMedia(mediaQueryString: string, defaultState?: boolean) { const features = useContext(MatchMediaContext) - const [matches, setMatches] = React.useState(() => { - if (features[mediaQueryString] !== undefined) { - return features[mediaQueryString] as boolean - } + // When the query is provided through `MatchMedia` context, that value always + // wins and there is nothing external to subscribe to. + const contextValue = features[mediaQueryString] as boolean | undefined - // Prevent a React hydration mismatch when a default value is provided by not defaulting to window.matchMedia(query).matches. - if (defaultState !== undefined) { - return defaultState - } + const subscribe = useCallback( + (onStoreChange: () => void) => { + if (contextValue !== undefined) { + return () => {} + } - if (canUseDOM) { - return window.matchMedia(mediaQueryString).matches - } + const mediaQueryList = window.matchMedia(mediaQueryString) - // A default value has not been provided, and you are rendering on the server, warn of a possible hydration mismatch when defaulting to false. - warning( - true, - '`useMedia` When server side rendering, defaultState should be defined to prevent a hydration mismatches.', - ) - - return false - }) + // Support fallback to `addListener` for broader browser support + // @ts-ignore this is not present in Safari <14 + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (mediaQueryList.addEventListener) { + mediaQueryList.addEventListener('change', onStoreChange) + return () => { + mediaQueryList.removeEventListener('change', onStoreChange) + } + } - if (features[mediaQueryString] !== undefined && matches !== features[mediaQueryString]) { - setMatches(features[mediaQueryString] as boolean) - } + mediaQueryList.addListener(onStoreChange) + return () => { + mediaQueryList.removeListener(onStoreChange) + } + }, + [contextValue, mediaQueryString], + ) - useEffect(() => { - // If `mediaQueryString` is present in features through `context` defer to - // the value present instead of checking with matchMedia - if (features[mediaQueryString] !== undefined) { - return + const getSnapshot = useCallback(() => { + if (contextValue !== undefined) { + return contextValue } + return window.matchMedia(mediaQueryString).matches + }, [contextValue, mediaQueryString]) - function listener(event: MediaQueryListEvent) { - setMatches(event.matches) + const getServerSnapshot = useCallback(() => { + if (contextValue !== undefined) { + return contextValue } - const mediaQueryList = window.matchMedia(mediaQueryString) - - // Support fallback to `addListener` for broader browser support - // @ts-ignore this is not present in Safari <14 - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - if (mediaQueryList.addEventListener) { - mediaQueryList.addEventListener('change', listener) - } else { - mediaQueryList.addListener(listener) + // Prevent a React hydration mismatch when a default value is provided by not + // defaulting to `window.matchMedia(query).matches`. + if (defaultState !== undefined) { + return defaultState } - // Make sure the media query list is in sync with the matches state - // eslint-disable-next-line react-hooks/set-state-in-effect - setMatches(mediaQueryList.matches) + // A default value has not been provided, and you are rendering on the + // server, warn of a possible hydration mismatch when defaulting to false. + warning( + true, + '`useMedia` When server side rendering, defaultState should be defined to prevent a hydration mismatches.', + ) - return () => { - // @ts-ignore this is not present in Safari <14 - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - if (mediaQueryList.addEventListener) { - mediaQueryList.removeEventListener('change', listener) - } else { - mediaQueryList.removeListener(listener) - } - } - }, [features, mediaQueryString]) + return false + }, [contextValue, defaultState]) - return matches + return useSyncExternalStore(subscribe, getSnapshot, getServerSnapshot) } export {MatchMedia} from './MatchMedia' diff --git a/packages/react/src/internal/components/ValidationAnimationContainer.tsx b/packages/react/src/internal/components/ValidationAnimationContainer.tsx index 6712307a357..c5b7ab52824 100644 --- a/packages/react/src/internal/components/ValidationAnimationContainer.tsx +++ b/packages/react/src/internal/components/ValidationAnimationContainer.tsx @@ -1,6 +1,6 @@ import type {HTMLProps} from 'react' import type React from 'react' -import {useEffect, useState} from 'react' +import {useState} from 'react' import classes from './ValidationAnimationContainer.module.css' interface Props extends HTMLProps { @@ -9,10 +9,12 @@ interface Props extends HTMLProps { const ValidationAnimationContainer: React.FC> = ({show, children}) => { const [shouldRender, setRender] = useState(show) - useEffect(() => { - // eslint-disable-next-line react-hooks/set-state-in-effect - if (show) setRender(true) - }, [show]) + // Mounting is derived from `show`, so compute it during render. Un-mounting is + // deferred to `onAnimationEnd` so the exit animation can play. Deriving this in + // render avoids the extra commit/paint an effect + setState would introduce. + if (show && !shouldRender) { + setRender(true) + } const onAnimationEnd = () => { if (!show) setRender(false) From 2ab8318e5675babe4836e12e4addf92fec5d8c46 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 16 Jun 2026 14:30:21 +0000 Subject: [PATCH 2/6] docs: justify remaining set-state-in-effect suppressions 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. --- packages/react/src/Autocomplete/AutocompleteMenu.tsx | 3 +++ packages/react/src/Dialog/Dialog.tsx | 1 + packages/react/src/LabelGroup/LabelGroup.tsx | 2 ++ packages/react/src/SelectPanel/SelectPanel.tsx | 6 ++++-- packages/react/src/TreeView/TreeView.tsx | 2 ++ .../react/src/experimental/SelectPanel2/SelectPanel.tsx | 2 ++ 6 files changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/react/src/Autocomplete/AutocompleteMenu.tsx b/packages/react/src/Autocomplete/AutocompleteMenu.tsx index 9d088ece9e9..a1f2bc49524 100644 --- a/packages/react/src/Autocomplete/AutocompleteMenu.tsx +++ b/packages/react/src/Autocomplete/AutocompleteMenu.tsx @@ -332,6 +332,9 @@ function AutocompleteMenu(props: AutocompleteMe itemIdSortResult.every((element, index) => element === sortedItemIds[index]) if (showMenu === false && !sortResultMatchesState) { + // Re-sort only when the menu closes. This effect also fires `onOpenChange` below, so it + // stays an effect. (Follow-up: the sort could be derived on the open→closed transition + // while keeping `onOpenChange` in an effect.) // eslint-disable-next-line react-hooks/set-state-in-effect setSortedItemIds(itemIdSortResult) } diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index 39e7cc36960..afdebac0dfe 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -492,6 +492,7 @@ const Buttons: React.FC> if (hasRendered === 1) { autoFocusRef.current?.focus() } else { + // Counts commits so focus is deferred by one render (a commit-timing hack), not derivable. // eslint-disable-next-line react-hooks/set-state-in-effect setHasRendered(hasRendered + 1) } diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index 541d29081af..227973313ca 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -245,6 +245,8 @@ const LabelGroup: React.FC> = ({ return () => observer.disconnect() } // We're not auto truncating, so we need to hide children after the given `visibleChildCount`. + // `hideChildrenAfterIndex` reads the rendered children from the DOM, so it must run in an + // effect. (Follow-up: this branch could derive the visibility map from child indices instead.) else { // eslint-disable-next-line react-hooks/set-state-in-effect hideChildrenAfterIndex(visibleChildCount) diff --git a/packages/react/src/SelectPanel/SelectPanel.tsx b/packages/react/src/SelectPanel/SelectPanel.tsx index 013a6e3c0e1..a5b67a4dedf 100644 --- a/packages/react/src/SelectPanel/SelectPanel.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.tsx @@ -388,7 +388,8 @@ function Panel({ useEffect(() => { if (open) { if (items.length === 0 && !(isLoading || loading)) { - // we need to wait for the listContainerElement to disappear before announcing no items, otherwise it will be interrupted + // We need to wait for the listContainerElement to disappear before announcing no items, + // otherwise it will be interrupted — this depends on commit timing, so it must run in an effect. // eslint-disable-next-line react-hooks/set-state-in-effect setNeedsNoItemsAnnouncement(true) } @@ -456,7 +457,8 @@ function Panel({ if (open) { // Only trigger filter change event if there are no items if (items.length === 0) { - // Trigger filter event to populate panel on first open + // Trigger filter event to populate panel on first open. This calls a consumer callback + // (a side effect), so it must run in an effect rather than during render. // eslint-disable-next-line react-hooks/set-state-in-effect onFilterChange(filterValue, null) } diff --git a/packages/react/src/TreeView/TreeView.tsx b/packages/react/src/TreeView/TreeView.tsx index 10a234b3dcc..ad3edf2d811 100644 --- a/packages/react/src/TreeView/TreeView.tsx +++ b/packages/react/src/TreeView/TreeView.tsx @@ -527,6 +527,8 @@ const SubTree: FCWithSlotMarker = ({count, state, children const parentElement = document.getElementById(itemId) if (!parentElement) return + // `getAccessibleName` reads the committed DOM, so the label cannot be derived during + // render and must be set from an effect. // eslint-disable-next-line react-hooks/set-state-in-effect setSubTreeLabel(getAccessibleName(parentElement)) if (previousState === 'loading' && state === 'done') { diff --git a/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx b/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx index e43442e5ced..6ba1ed8763d 100644 --- a/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx +++ b/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx @@ -328,6 +328,8 @@ const SelectPanelButton = React.forwardRef((prop useEffect(() => { const label = document.querySelector(`[for='${inputProps.id}']`) if (label?.textContent) { + // Reads the associated label's `textContent` from the committed DOM, so it must run in + // an effect. // eslint-disable-next-line react-hooks/set-state-in-effect setLabelText(label.textContent) } From b804c7453b873518c0f3037ca975014a8d7c1a8c Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 23 Jun 2026 07:59:55 -0400 Subject: [PATCH 3/6] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .changeset/derive-state-from-effects.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/derive-state-from-effects.md b/.changeset/derive-state-from-effects.md index 19c9ae562f4..d0003f1e5e6 100644 --- a/.changeset/derive-state-from-effects.md +++ b/.changeset/derive-state-from-effects.md @@ -2,4 +2,4 @@ '@primer/react': patch --- -Derive state during render instead of in effects for `useMedia`, `ToggleSwitch`, `SelectPanel`, and the internal validation animation container, removing redundant render passes (no public API changes). +Derive state during render instead of in effects for `useMedia`, `ToggleSwitch`, `SelectPanel`, and the internal validation animation container, removing redundant render passes. Note: `useMedia` now returns the live `matchMedia` value on the first client render; `defaultState` is only used for the SSR/hydration snapshot (no public API changes). From 37371f25b0624bec3de50eaab34a02416a6c893c Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 23 Jun 2026 14:51:38 +0000 Subject: [PATCH 4/6] test(useMedia): assert expected SSR hydration warning --- packages/react/src/hooks/__tests__/useMedia.test.tsx | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/react/src/hooks/__tests__/useMedia.test.tsx b/packages/react/src/hooks/__tests__/useMedia.test.tsx index 2a4684b08c7..6ba31b5a4a9 100644 --- a/packages/react/src/hooks/__tests__/useMedia.test.tsx +++ b/packages/react/src/hooks/__tests__/useMedia.test.tsx @@ -68,6 +68,7 @@ describe('useMedia', () => { }) it('should default to false when used during SSR', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) const match: boolean[] = [] function TestComponent() { @@ -78,6 +79,10 @@ describe('useMedia', () => { ReactDOM.renderToString() expect(match[0]).toBe(false) + // Without a `defaultState`, rendering on the server warns about a possible + // hydration mismatch. + expect(warnSpy).toHaveBeenCalledWith('Warning:', expect.stringContaining('`useMedia` When server side rendering')) + warnSpy.mockRestore() }) it('should respond to change in matchMedia values', () => { From a475aef32e96ce1251f83b7f3f3e292e156bf69f Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 7 Jul 2026 15:28:53 +0000 Subject: [PATCH 5/6] fix(useMedia): only warn about missing defaultState on the server, not during client hydration; keep SelectPanel intermediate-reset as render-time state (ref writes disallowed in render) --- .../react/src/SelectPanel/SelectPanel.tsx | 6 +++-- .../src/hooks/__tests__/useMedia.test.tsx | 25 ++++++++++++++++--- packages/react/src/hooks/useMedia.ts | 6 ++++- 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/packages/react/src/SelectPanel/SelectPanel.tsx b/packages/react/src/SelectPanel/SelectPanel.tsx index 56ee59c8daf..9555348297a 100644 --- a/packages/react/src/SelectPanel/SelectPanel.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.tsx @@ -246,8 +246,10 @@ function Panel({ ) // Reset the intermediate selected item when the panel is opened or closed. - // Tracking the previous `open` value lets us derive this during render instead - // of in an effect, avoiding an extra render pass each time the panel toggles. + // Tracking the previous `open` value in state lets us derive this during render + // instead of in an effect. Adjusting state during render this way does not cause + // an extra committed render — React re-renders synchronously before painting — + // and a ref cannot be used here because writing refs during render is disallowed. const [prevOpen, setPrevOpen] = useState(open) if (prevOpen !== open) { setPrevOpen(open) diff --git a/packages/react/src/hooks/__tests__/useMedia.test.tsx b/packages/react/src/hooks/__tests__/useMedia.test.tsx index 6ba31b5a4a9..a2579704eb2 100644 --- a/packages/react/src/hooks/__tests__/useMedia.test.tsx +++ b/packages/react/src/hooks/__tests__/useMedia.test.tsx @@ -68,7 +68,6 @@ describe('useMedia', () => { }) it('should default to false when used during SSR', () => { - const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) const match: boolean[] = [] function TestComponent() { @@ -77,11 +76,29 @@ describe('useMedia', () => { return null } + // `renderToString` uses the server snapshot, which defaults to `false` when + // no `defaultState` is provided. ReactDOM.renderToString() expect(match[0]).toBe(false) - // Without a `defaultState`, rendering on the server warns about a possible - // hydration mismatch. - expect(warnSpy).toHaveBeenCalledWith('Warning:', expect.stringContaining('`useMedia` When server side rendering')) + }) + + it('does not warn about a missing defaultState on the client', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + function TestComponent() { + useMedia('(pointer: coarse)') + return null + } + + // On the client (where `window` is defined) the hook reads `matchMedia` + // directly, so the SSR guidance warning must not appear in the browser + // console — including during hydration, when `getServerSnapshot` runs. + render() + + expect(warnSpy).not.toHaveBeenCalledWith( + 'Warning:', + expect.stringContaining('`useMedia` When server side rendering'), + ) warnSpy.mockRestore() }) diff --git a/packages/react/src/hooks/useMedia.ts b/packages/react/src/hooks/useMedia.ts index b818b0adb1f..abe36d44ced 100644 --- a/packages/react/src/hooks/useMedia.ts +++ b/packages/react/src/hooks/useMedia.ts @@ -67,8 +67,12 @@ export function useMedia(mediaQueryString: string, defaultState?: boolean) { // A default value has not been provided, and you are rendering on the // server, warn of a possible hydration mismatch when defaulting to false. + // `getServerSnapshot` also runs on the client during hydration; only warn on + // the actual server so we don't surface this message in the browser console + // (matching the previous behavior, where the client read `matchMedia` and + // stayed quiet). warning( - true, + typeof window === 'undefined', '`useMedia` When server side rendering, defaultState should be defined to prevent a hydration mismatches.', ) From 7358e8ce22a000d0e0ce8d203c7c82d3f9360c8d Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 7 Jul 2026 20:20:31 +0000 Subject: [PATCH 6/6] refactor(useMedia): scope PR to useMedia useSyncExternalStore refactor Drop the overlapping ToggleSwitch/SelectPanel/ValidationAnimationContainer changes (handled by #8024/#8025/#8023) and the comment-only edits to Autocomplete/Dialog/LabelGroup/TreeView/SelectPanel2. Retarget the changeset to the useMedia change only. --- .changeset/derive-state-from-effects.md | 5 ----- .changeset/usemedia-sync-external-store.md | 5 +++++ .../src/Autocomplete/AutocompleteMenu.tsx | 3 --- packages/react/src/Dialog/Dialog.tsx | 1 - packages/react/src/LabelGroup/LabelGroup.tsx | 2 -- .../react/src/SelectPanel/SelectPanel.tsx | 19 ++++++------------- .../react/src/ToggleSwitch/ToggleSwitch.tsx | 14 +++++--------- packages/react/src/TreeView/TreeView.tsx | 2 -- .../experimental/SelectPanel2/SelectPanel.tsx | 2 -- .../ValidationAnimationContainer.tsx | 12 +++++------- 10 files changed, 21 insertions(+), 44 deletions(-) delete mode 100644 .changeset/derive-state-from-effects.md create mode 100644 .changeset/usemedia-sync-external-store.md diff --git a/.changeset/derive-state-from-effects.md b/.changeset/derive-state-from-effects.md deleted file mode 100644 index d0003f1e5e6..00000000000 --- a/.changeset/derive-state-from-effects.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@primer/react': patch ---- - -Derive state during render instead of in effects for `useMedia`, `ToggleSwitch`, `SelectPanel`, and the internal validation animation container, removing redundant render passes. Note: `useMedia` now returns the live `matchMedia` value on the first client render; `defaultState` is only used for the SSR/hydration snapshot (no public API changes). diff --git a/.changeset/usemedia-sync-external-store.md b/.changeset/usemedia-sync-external-store.md new file mode 100644 index 00000000000..d63dd379fd6 --- /dev/null +++ b/.changeset/usemedia-sync-external-store.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +`useMedia` now reads the live `matchMedia` value on the first client render instead of after an effect, removing a redundant render pass. `defaultState` is only used for the SSR/hydration snapshot (no public API changes). diff --git a/packages/react/src/Autocomplete/AutocompleteMenu.tsx b/packages/react/src/Autocomplete/AutocompleteMenu.tsx index b87472c3149..57ecf08420c 100644 --- a/packages/react/src/Autocomplete/AutocompleteMenu.tsx +++ b/packages/react/src/Autocomplete/AutocompleteMenu.tsx @@ -334,9 +334,6 @@ function AutocompleteMenu(props: AutocompleteMe // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler if (showMenu === false && !sortResultMatchesState) { - // Re-sort only when the menu closes. This effect also fires `onOpenChange` below, so it - // stays an effect. (Follow-up: the sort could be derived on the open→closed transition - // while keeping `onOpenChange` in an effect.) // eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-derived-state setSortedItemIds(itemIdSortResult) } diff --git a/packages/react/src/Dialog/Dialog.tsx b/packages/react/src/Dialog/Dialog.tsx index c9ab4c392fd..de9cc4f8794 100644 --- a/packages/react/src/Dialog/Dialog.tsx +++ b/packages/react/src/Dialog/Dialog.tsx @@ -492,7 +492,6 @@ const Buttons: React.FC> if (hasRendered === 1) { autoFocusRef.current?.focus() } else { - // Counts commits so focus is deferred by one render (a commit-timing hack), not derivable. // eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-derived-state setHasRendered(hasRendered + 1) } diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index fa91230cb56..3fb0727b03f 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -246,8 +246,6 @@ const LabelGroup: React.FC> = ({ return () => observer.disconnect() } // We're not auto truncating, so we need to hide children after the given `visibleChildCount`. - // `hideChildrenAfterIndex` reads the rendered children from the DOM, so it must run in an - // effect. (Follow-up: this branch could derive the visibility map from child indices instead.) else { // eslint-disable-next-line react-hooks/set-state-in-effect hideChildrenAfterIndex(visibleChildCount) diff --git a/packages/react/src/SelectPanel/SelectPanel.tsx b/packages/react/src/SelectPanel/SelectPanel.tsx index 9555348297a..d818724c193 100644 --- a/packages/react/src/SelectPanel/SelectPanel.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.tsx @@ -245,16 +245,11 @@ function Panel({ isSingleSelectModal ? selected : undefined, ) - // Reset the intermediate selected item when the panel is opened or closed. - // Tracking the previous `open` value in state lets us derive this during render - // instead of in an effect. Adjusting state during render this way does not cause - // an extra committed render — React re-renders synchronously before painting — - // and a ref cannot be used here because writing refs during render is disallowed. - const [prevOpen, setPrevOpen] = useState(open) - if (prevOpen !== open) { - setPrevOpen(open) + // Reset the intermediate selected item when the panel is open/closed + useEffect(() => { + // eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-derived-state setIntermediateSelected(isSingleSelectModal ? selected : undefined) - } + }, [isSingleSelectModal, open, selected]) const onListContainerRefChanged: FilteredActionListProps['onListContainerRefChanged'] = useCallback( (node: HTMLElement | null) => { @@ -397,8 +392,7 @@ function Panel({ if (open) { // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler if (items.length === 0 && !(isLoading || loading)) { - // We need to wait for the listContainerElement to disappear before announcing no items, - // otherwise it will be interrupted — this depends on commit timing, so it must run in an effect. + // we need to wait for the listContainerElement to disappear before announcing no items, otherwise it will be interrupted // eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-adjust-state-on-prop-change setNeedsNoItemsAnnouncement(true) } @@ -479,8 +473,7 @@ function Panel({ // Only trigger filter change event if there are no items // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler if (items.length === 0) { - // Trigger filter event to populate panel on first open. This calls a consumer callback - // (a side effect), so it must run in an effect rather than during render. + // Trigger filter event to populate panel on first open // eslint-disable-next-line react-hooks/set-state-in-effect onFilterChange(filterValue, null) } diff --git a/packages/react/src/ToggleSwitch/ToggleSwitch.tsx b/packages/react/src/ToggleSwitch/ToggleSwitch.tsx index 5ef4616fc93..ebe8e7fc512 100644 --- a/packages/react/src/ToggleSwitch/ToggleSwitch.tsx +++ b/packages/react/src/ToggleSwitch/ToggleSwitch.tsx @@ -123,16 +123,12 @@ const ToggleSwitch = React.forwardRef(func } }, [onChange, checked, isControlled, disabled]) - // Hiding the loading label is pure state derived from `loading`, so compute it - // during render. Only the delayed reveal needs a timer, which stays in the - // effect below. - if (!loading && isLoadingLabelVisible) { - setIsLoadingLabelVisible(false) - } - useEffect(() => { - // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler - if (loading && !isLoadingLabelVisible) { + if (!loading && isLoadingLabelVisible) { + // eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-chain-state-updates + setIsLoadingLabelVisible(false) + // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler + } else if (loading && !isLoadingLabelVisible) { safeSetTimeout(() => { setIsLoadingLabelVisible(true) }, loadingLabelDelay) diff --git a/packages/react/src/TreeView/TreeView.tsx b/packages/react/src/TreeView/TreeView.tsx index eceaa5acfa9..3338c6371d2 100644 --- a/packages/react/src/TreeView/TreeView.tsx +++ b/packages/react/src/TreeView/TreeView.tsx @@ -556,8 +556,6 @@ const SubTree: FCWithSlotMarker = ({count, state, children const parentElement = document.getElementById(itemId) if (!parentElement) return - // `getAccessibleName` reads the committed DOM, so the label cannot be derived during - // render and must be set from an effect. // eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-chain-state-updates setSubTreeLabel(getAccessibleName(parentElement)) if (previousState === 'loading' && state === 'done') { diff --git a/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx b/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx index f6900b8d117..a94b0f009fa 100644 --- a/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx +++ b/packages/react/src/experimental/SelectPanel2/SelectPanel.tsx @@ -329,8 +329,6 @@ const SelectPanelButton = React.forwardRef((prop useEffect(() => { const label = document.querySelector(`[for='${inputProps.id}']`) if (label?.textContent) { - // Reads the associated label's `textContent` from the committed DOM, so it must run in - // an effect. // eslint-disable-next-line react-hooks/set-state-in-effect setLabelText(label.textContent) } diff --git a/packages/react/src/internal/components/ValidationAnimationContainer.tsx b/packages/react/src/internal/components/ValidationAnimationContainer.tsx index c5b7ab52824..637502770f2 100644 --- a/packages/react/src/internal/components/ValidationAnimationContainer.tsx +++ b/packages/react/src/internal/components/ValidationAnimationContainer.tsx @@ -1,6 +1,6 @@ import type {HTMLProps} from 'react' import type React from 'react' -import {useState} from 'react' +import {useEffect, useState} from 'react' import classes from './ValidationAnimationContainer.module.css' interface Props extends HTMLProps { @@ -9,12 +9,10 @@ interface Props extends HTMLProps { const ValidationAnimationContainer: React.FC> = ({show, children}) => { const [shouldRender, setRender] = useState(show) - // Mounting is derived from `show`, so compute it during render. Un-mounting is - // deferred to `onAnimationEnd` so the exit animation can play. Deriving this in - // render avoids the extra commit/paint an effect + setState would introduce. - if (show && !shouldRender) { - setRender(true) - } + useEffect(() => { + // eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-derived-state, react-you-might-not-need-an-effect/no-event-handler + if (show) setRender(true) + }, [show]) const onAnimationEnd = () => { if (!show) setRender(false)