From 4e6cf0d1715e0371169b291be7a503f2d15f7bc5 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 16 Jun 2026 14:01:02 +0000 Subject: [PATCH 01/10] 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 02/10] 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 533376392ec49d2b1570a9897a84b70db8871c02 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 17 Jun 2026 12:30:28 +0000 Subject: [PATCH 03/10] Initial plan From 7b943151e8b61d6bd97b86e4cd8d2e1a391fd6e8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 17 Jun 2026 12:43:22 +0000 Subject: [PATCH 04/10] refactor: derive follow-up state during render Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com> --- .../src/Autocomplete/Autocomplete.test.tsx | 19 ++++-- .../src/Autocomplete/AutocompleteMenu.tsx | 28 ++++---- .../react/src/LabelGroup/LabelGroup.test.tsx | 17 ++++- packages/react/src/LabelGroup/LabelGroup.tsx | 64 ++++++++++++------- 4 files changed, 84 insertions(+), 44 deletions(-) diff --git a/packages/react/src/Autocomplete/Autocomplete.test.tsx b/packages/react/src/Autocomplete/Autocomplete.test.tsx index 054a9b52e0a..ae93bb6f807 100644 --- a/packages/react/src/Autocomplete/Autocomplete.test.tsx +++ b/packages/react/src/Autocomplete/Autocomplete.test.tsx @@ -317,22 +317,29 @@ describe('Autocomplete', () => { }) it("calls onOpenChange with the menu's open state", async () => { - const user = userEvent.setup() const onOpenChangeMock = vi.fn() - const {container} = render( + const {getByLabelText} = render( , ) - const inputNode = container.querySelector('#autocompleteInput') + const inputNode = getByLabelText(AUTOCOMPLETE_LABEL) - inputNode && (await user.type(inputNode, 'ze')) - expect(onOpenChangeMock).toHaveBeenCalled() + expect(onOpenChangeMock).toHaveBeenNthCalledWith(1, false) + + fireEvent.click(inputNode) + fireEvent.keyDown(inputNode, {key: 'ArrowDown'}) + await waitFor(() => expect(onOpenChangeMock).toHaveBeenLastCalledWith(true)) + + // eslint-disable-next-line github/no-blur + fireEvent.blur(inputNode) + + await waitFor(() => expect(onOpenChangeMock).toHaveBeenLastCalledWith(false)) }) it('calls onSelectedChange with the data for the selected items', async () => { diff --git a/packages/react/src/Autocomplete/AutocompleteMenu.tsx b/packages/react/src/Autocomplete/AutocompleteMenu.tsx index b87472c3149..357a4722561 100644 --- a/packages/react/src/Autocomplete/AutocompleteMenu.tsx +++ b/packages/react/src/Autocomplete/AutocompleteMenu.tsx @@ -169,6 +169,7 @@ function AutocompleteMenu(props: AutocompleteMe const allItemsToRenderRef = useRef([]) const [highlightedItem, setHighlightedItem] = useState() const [sortedItemIds, setSortedItemIds] = useState>(items.map(({id: itemId}) => itemId)) + const [prevShowMenu, setPrevShowMenu] = useState(showMenu) const generatedUniqueId = useId(id) const selectableItems = useMemo( @@ -324,26 +325,25 @@ function AutocompleteMenu(props: AutocompleteMe } }, [highlightedItem, deferredInputValue, selectedItemIds, setAutocompleteSuggestion]) - useEffect(() => { - const itemIdSortResult = [...sortedItemIds].sort( - sortOnCloseFn ? sortOnCloseFn : getDefaultSortFn(itemId => isItemSelected(itemId, selectedItemIds)), - ) - const sortResultMatchesState = - itemIdSortResult.length === sortedItemIds.length && - itemIdSortResult.every((element, index) => element === sortedItemIds[index]) + const itemIdSortResult = [...sortedItemIds].sort( + sortOnCloseFn ? sortOnCloseFn : getDefaultSortFn(itemId => isItemSelected(itemId, selectedItemIds)), + ) + const sortResultMatchesState = + itemIdSortResult.length === sortedItemIds.length && + itemIdSortResult.every((element, index) => element === sortedItemIds[index]) - // 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 + if (prevShowMenu !== showMenu) { + setPrevShowMenu(showMenu) + + if (prevShowMenu && showMenu === false && !sortResultMatchesState) { setSortedItemIds(itemIdSortResult) } + } + useEffect(() => { // eslint-disable-next-line react-you-might-not-need-an-effect/no-pass-data-to-parent onOpenChange && onOpenChange(Boolean(showMenu)) - }, [showMenu, onOpenChange, selectedItemIds, sortOnCloseFn, sortedItemIds]) + }, [showMenu, onOpenChange]) useEffect(() => { // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler diff --git a/packages/react/src/LabelGroup/LabelGroup.test.tsx b/packages/react/src/LabelGroup/LabelGroup.test.tsx index 2e141fc6d78..f22715d3654 100644 --- a/packages/react/src/LabelGroup/LabelGroup.test.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.test.tsx @@ -111,7 +111,7 @@ describe('LabelGroup', () => { }) it('should truncate labels to a specified number', () => { - const {getByText} = render( + const {getByText, queryByText, rerender} = render( @@ -125,6 +125,21 @@ describe('LabelGroup', () => { const expandButton = getByText('+2') expect(expandButton).toBeDefined() + + rerender( + + + + + + + + + , + ) + + expect(queryByText('+2')).toBeNull() + expect(getByText('+1')).toBeDefined() }) it('should expand all tokens into an overlay when overflowStyle="overlay"', async () => { diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index fa91230cb56..f40667806f7 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -23,6 +23,26 @@ const getOverlayWidth = ( overlayPaddingPx: number, ) => overlayPaddingPx + buttonClientRect.right - (containerRef.current?.getBoundingClientRect().left || 0) +const getVisibilityMapAfterIndex = (childCount: number, truncateAfter: number) => { + const updatedEntries: Record = {} + + for (let index = 0; index < childCount; index++) { + updatedEntries[index.toString()] = index < truncateAfter + } + + return updatedEntries +} + +const visibilityMapsMatch = (left: Record, right: Record) => { + const leftKeys = Object.keys(left) + const rightKeys = Object.keys(right) + + return ( + leftKeys.length === rightKeys.length && + leftKeys.every(key => Object.prototype.hasOwnProperty.call(right, key) && left[key] === right[key]) + ) +} + const InlineToggle: React.FC<{ collapseButtonRef: React.RefObject collapseInlineExpandedChildren: () => void @@ -131,6 +151,7 @@ const LabelGroup: React.FC> = ({ bottom: 0, toJSON: () => undefined, }) + const childArray = React.Children.toArray(children) const overlayPaddingPx = 8 // var(--base-size-8), hardcoded to do some math const hiddenItemIds = Object.keys(visibilityMap).filter(key => !visibilityMap[key]) @@ -163,18 +184,12 @@ const LabelGroup: React.FC> = ({ ) // Sets the visibility map to hide children after the given index. - const hideChildrenAfterIndex = React.useCallback((truncateAfter: number) => { - const containerChildren = containerRef.current?.children || [] - const updatedEntries: Record = {} - for (const child of containerChildren) { - const targetId = child.getAttribute('data-index') - if (targetId) { - updatedEntries[targetId] = parseInt(targetId, 10) < truncateAfter - } - } - - setVisibilityMap(updatedEntries) - }, []) + const hideChildrenAfterIndex = React.useCallback( + (truncateAfter: number) => { + setVisibilityMap(getVisibilityMapAfterIndex(childArray.length, truncateAfter)) + }, + [childArray.length], + ) const openOverflowOverlay = React.useCallback(() => setIsOverflowShown(true), [setIsOverflowShown]) @@ -203,6 +218,15 @@ const LabelGroup: React.FC> = ({ setIsOverflowShown(true) }, [setVisibilityMap, setIsOverflowShown]) + const numericVisibilityMap = + typeof visibleChildCount === 'number' && !isOverflowShown + ? getVisibilityMapAfterIndex(childArray.length, visibleChildCount) + : undefined + + if (numericVisibilityMap && !visibilityMapsMatch(visibilityMap, numericVisibilityMap)) { + setVisibilityMap(numericVisibilityMap) + } + React.useEffect(() => { // If we're not truncating, we don't need to run this useEffect. // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler @@ -210,6 +234,7 @@ const LabelGroup: React.FC> = ({ return } + // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler if (visibleChildCount === 'auto') { // Instantiates the IntersectionObserver to track when children fit in the container. const observer = new IntersectionObserver( @@ -245,14 +270,7 @@ 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) - } - }, [buttonClientRect, visibleChildCount, hideChildrenAfterIndex, isOverflowShown]) + }, [buttonClientRect, visibleChildCount, isOverflowShown]) // Updates the index of the first hidden child. // We need to keep track of this so we can focus the first hidden child when the overflow is shown inline. @@ -304,7 +322,7 @@ const LabelGroup: React.FC> = ({ className={clsx(className, classes.Container)} data-component="LabelGroup" > - {React.Children.map(children, (child, index) => ( + {childArray.map((child, index) => ( > = ({ hiddenItemIds={hiddenItemIds} isOverflowShown={isOverflowShown} showAllTokensInline={showAllTokensInline} - totalLength={React.Children.toArray(children).length} + totalLength={childArray.length} /> ) : ( > = ({ openOverflowOverlay={openOverflowOverlay} overlayPaddingPx={overlayPaddingPx} overlayWidth={overlayWidth} - totalLength={React.Children.toArray(children).length} + totalLength={childArray.length} > {children} From cfb76f4f9b81553ac4bd68824f0f6e9f3141b1f1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 17 Jun 2026 12:51:03 +0000 Subject: [PATCH 05/10] refactor: finalize render-time state derivation Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com> --- packages/react/src/LabelGroup/LabelGroup.tsx | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index f40667806f7..15017e1facd 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -218,10 +218,13 @@ const LabelGroup: React.FC> = ({ setIsOverflowShown(true) }, [setVisibilityMap, setIsOverflowShown]) - const numericVisibilityMap = - typeof visibleChildCount === 'number' && !isOverflowShown - ? getVisibilityMapAfterIndex(childArray.length, visibleChildCount) - : undefined + const numericVisibilityMap = React.useMemo( + () => + typeof visibleChildCount === 'number' && !isOverflowShown + ? getVisibilityMapAfterIndex(childArray.length, visibleChildCount) + : undefined, + [childArray.length, isOverflowShown, visibleChildCount], + ) if (numericVisibilityMap && !visibilityMapsMatch(visibilityMap, numericVisibilityMap)) { setVisibilityMap(numericVisibilityMap) From b804c7453b873518c0f3037ca975014a8d7c1a8c Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 23 Jun 2026 07:59:55 -0400 Subject: [PATCH 06/10] 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 07/10] 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 5d24be483f2fdfa305970907984d04522e2aa13c Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 23 Jun 2026 15:23:16 +0000 Subject: [PATCH 08/10] fix(Autocomplete): derive close-time sort safely despite menu remount; address review feedback - AutocompleteMenu remounts on open/close (Overlay vs VisuallyHidden parent), so the prevShowMenu render transition never fired and the re-sort was dead code. Derive the sorted order during render while the menu is closed instead. - LabelGroup: align numeric visibility guard with existing truthiness checks so visibleChildCount={0} no longer causes redundant render-phase updates. - Add changeset for the Autocomplete/LabelGroup render-time derivation. --- ...autocomplete-labelgroup-state-in-render.md | 5 ++++ .../src/Autocomplete/Autocomplete.test.tsx | 26 ++++++++++-------- .../src/Autocomplete/AutocompleteMenu.tsx | 27 ++++++++++--------- packages/react/src/LabelGroup/LabelGroup.tsx | 2 +- 4 files changed, 36 insertions(+), 24 deletions(-) create mode 100644 .changeset/derive-autocomplete-labelgroup-state-in-render.md diff --git a/.changeset/derive-autocomplete-labelgroup-state-in-render.md b/.changeset/derive-autocomplete-labelgroup-state-in-render.md new file mode 100644 index 00000000000..67cfa116ac1 --- /dev/null +++ b/.changeset/derive-autocomplete-labelgroup-state-in-render.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Derive `AutocompleteMenu` close-time re-sorting and `LabelGroup` numeric truncation during render instead of in effects, removing redundant render passes. No public API or visual changes. diff --git a/packages/react/src/Autocomplete/Autocomplete.test.tsx b/packages/react/src/Autocomplete/Autocomplete.test.tsx index ae93bb6f807..7caf9d195a8 100644 --- a/packages/react/src/Autocomplete/Autocomplete.test.tsx +++ b/packages/react/src/Autocomplete/Autocomplete.test.tsx @@ -289,30 +289,34 @@ describe('Autocomplete', () => { }) it('calls a custom sort function when the menu closes', async () => { - const user = userEvent.setup() const sortOnCloseFnMock = vi.fn() + const onOpenChangeMock = vi.fn() const {container} = render( , ) - const inputNode = container.querySelector('#autocompleteInput') + const inputNode = container.querySelector('#autocompleteInput') as HTMLInputElement - // `sortOnCloseFnMock` will be called in a `.sort()` on render to check if the - // current sort order matches the result of `sortOnCloseFnMock` - expect(sortOnCloseFnMock).toHaveBeenCalledTimes(mockItems.length - 1) - if (inputNode) { - await user.type(inputNode, 'ze') - // eslint-disable-next-line github/no-blur - fireEvent.blur(inputNode) - } + // The menu starts closed, so the items are sorted during render. + expect(sortOnCloseFnMock).toHaveBeenCalled() - // wait a tick for blur to finish + // Opening the menu does not run the close-time sort. + sortOnCloseFnMock.mockClear() + fireEvent.click(inputNode) + fireEvent.keyDown(inputNode, {key: 'ArrowDown'}) + await waitFor(() => expect(onOpenChangeMock).toHaveBeenLastCalledWith(true)) + expect(sortOnCloseFnMock).not.toHaveBeenCalled() + + // Closing the menu re-runs the sort. + // eslint-disable-next-line github/no-blur + fireEvent.blur(inputNode) await waitFor(() => expect(sortOnCloseFnMock).toHaveBeenCalled()) }) diff --git a/packages/react/src/Autocomplete/AutocompleteMenu.tsx b/packages/react/src/Autocomplete/AutocompleteMenu.tsx index 357a4722561..672be5835cc 100644 --- a/packages/react/src/Autocomplete/AutocompleteMenu.tsx +++ b/packages/react/src/Autocomplete/AutocompleteMenu.tsx @@ -169,7 +169,6 @@ function AutocompleteMenu(props: AutocompleteMe const allItemsToRenderRef = useRef([]) const [highlightedItem, setHighlightedItem] = useState() const [sortedItemIds, setSortedItemIds] = useState>(items.map(({id: itemId}) => itemId)) - const [prevShowMenu, setPrevShowMenu] = useState(showMenu) const generatedUniqueId = useId(id) const selectableItems = useMemo( @@ -325,17 +324,21 @@ function AutocompleteMenu(props: AutocompleteMe } }, [highlightedItem, deferredInputValue, selectedItemIds, setAutocompleteSuggestion]) - const itemIdSortResult = [...sortedItemIds].sort( - sortOnCloseFn ? sortOnCloseFn : getDefaultSortFn(itemId => isItemSelected(itemId, selectedItemIds)), - ) - const sortResultMatchesState = - itemIdSortResult.length === sortedItemIds.length && - itemIdSortResult.every((element, index) => element === sortedItemIds[index]) - - if (prevShowMenu !== showMenu) { - setPrevShowMenu(showMenu) - - if (prevShowMenu && showMenu === false && !sortResultMatchesState) { + // Re-sort the items during render while the menu is closed so the order is ready the next time + // it opens. `AutocompleteOverlay` mounts this component under a different parent when the menu + // opens vs. closes, so it remounts on each open/close and a render-time previous-value transition + // can't be observed here — guarding on the closed state is what replaces the old close-time effect. + // The comparator therefore runs only while the menu is closed, never on the frequent renders that + // happen while it is open. + if (showMenu === false) { + const itemIdSortResult = [...sortedItemIds].sort( + sortOnCloseFn ? sortOnCloseFn : getDefaultSortFn(itemId => isItemSelected(itemId, selectedItemIds)), + ) + const sortResultMatchesState = + itemIdSortResult.length === sortedItemIds.length && + itemIdSortResult.every((element, index) => element === sortedItemIds[index]) + + if (!sortResultMatchesState) { setSortedItemIds(itemIdSortResult) } } diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index 15017e1facd..b3fe7a5b133 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -220,7 +220,7 @@ const LabelGroup: React.FC> = ({ const numericVisibilityMap = React.useMemo( () => - typeof visibleChildCount === 'number' && !isOverflowShown + visibleChildCount && typeof visibleChildCount === 'number' && !isOverflowShown ? getVisibilityMapAfterIndex(childArray.length, visibleChildCount) : undefined, [childArray.length, isOverflowShown, visibleChildCount], From a475aef32e96ce1251f83b7f3f3e292e156bf69f Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 7 Jul 2026 15:28:53 +0000 Subject: [PATCH 09/10] 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 702f528f5918ece7eb16b49d741e2eaa9a044c61 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Tue, 7 Jul 2026 21:40:21 +0000 Subject: [PATCH 10/10] refactor: scope PR to Autocomplete + LabelGroup render-derivation 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. --- .changeset/derive-state-from-effects.md | 5 - packages/react/src/Dialog/Dialog.tsx | 1 - .../react/src/SelectPanel/SelectPanel.tsx | 19 +--- .../react/src/ToggleSwitch/ToggleSwitch.tsx | 14 +-- packages/react/src/TreeView/TreeView.tsx | 2 - .../experimental/SelectPanel2/SelectPanel.tsx | 2 - .../src/hooks/__tests__/useMedia.test.tsx | 28 +---- packages/react/src/hooks/useMedia.ts | 105 +++++++++--------- .../ValidationAnimationContainer.tsx | 12 +- 9 files changed, 71 insertions(+), 117 deletions(-) delete mode 100644 .changeset/derive-state-from-effects.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/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/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/hooks/__tests__/useMedia.test.tsx b/packages/react/src/hooks/__tests__/useMedia.test.tsx index a2579704eb2..2884c18f2da 100644 --- a/packages/react/src/hooks/__tests__/useMedia.test.tsx +++ b/packages/react/src/hooks/__tests__/useMedia.test.tsx @@ -8,14 +8,11 @@ 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: currentMatches, + matches: defaultMatch, media: query, onchange: null, addListener: vi.fn(), // deprecated @@ -32,7 +29,6 @@ function mockMatchMedia({defaultMatch = false} = {}) { return { change({matches = false}) { - currentMatches = matches for (const listener of listeners) { listener({ matches, @@ -76,32 +72,10 @@ 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) }) - 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() - }) - it('should respond to change in matchMedia values', () => { const {change} = mockMatchMedia() diff --git a/packages/react/src/hooks/useMedia.ts b/packages/react/src/hooks/useMedia.ts index abe36d44ced..418949a1e7f 100644 --- a/packages/react/src/hooks/useMedia.ts +++ b/packages/react/src/hooks/useMedia.ts @@ -1,4 +1,5 @@ -import {useCallback, useContext, useSyncExternalStore} from 'react' +import React, {useContext, useEffect} from 'react' +import {canUseDOM} from '../utils/environment' import {warning} from '../utils/warning' import {MatchMediaContext} from './MatchMediaContext' @@ -17,69 +18,71 @@ import {MatchMediaContext} from './MatchMediaContext' */ export function useMedia(mediaQueryString: string, defaultState?: boolean) { const features = useContext(MatchMediaContext) - // 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 + const [matches, setMatches] = React.useState(() => { + if (features[mediaQueryString] !== undefined) { + return features[mediaQueryString] as boolean + } - const subscribe = useCallback( - (onStoreChange: () => void) => { - if (contextValue !== undefined) { - return () => {} - } + // 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 mediaQueryList = window.matchMedia(mediaQueryString) + if (canUseDOM) { + return window.matchMedia(mediaQueryString).matches + } - // 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) - } - } + // 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.', + ) - mediaQueryList.addListener(onStoreChange) - return () => { - mediaQueryList.removeListener(onStoreChange) - } - }, - [contextValue, mediaQueryString], - ) + return false + }) + + if (features[mediaQueryString] !== undefined && matches !== features[mediaQueryString]) { + setMatches(features[mediaQueryString] as boolean) + } - const getSnapshot = useCallback(() => { - if (contextValue !== undefined) { - return contextValue + useEffect(() => { + // If `mediaQueryString` is present in features through `context` defer to + // the value present instead of checking with matchMedia + if (features[mediaQueryString] !== undefined) { + return } - return window.matchMedia(mediaQueryString).matches - }, [contextValue, mediaQueryString]) - const getServerSnapshot = useCallback(() => { - if (contextValue !== undefined) { - return contextValue + function listener(event: MediaQueryListEvent) { + setMatches(event.matches) } - // 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 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) } - // 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( - typeof window === 'undefined', - '`useMedia` When server side rendering, defaultState should be defined to prevent a hydration mismatches.', - ) + // Make sure the media query list is in sync with the matches state + // eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-external-store-subscription + setMatches(mediaQueryList.matches) - return false - }, [contextValue, defaultState]) + 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 useSyncExternalStore(subscribe, getSnapshot, getServerSnapshot) + return matches } export {MatchMedia} from './MatchMedia' 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)