From e2e7d7712b79dfe1c3c60d8b461e55a0e7cd8c58 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Mon, 22 Jun 2026 13:01:45 +0000 Subject: [PATCH 1/3] perf(FormControl): derive validation message visibility during render Replace the effect that set ValidationAnimationContainer's shouldRender with an adjust-during-render update, removing the extra post-commit render when the validation message appears. Adds a render-count regression test. --- .../validation-animation-derive-in-render.md | 5 ++ .../ValidationAnimationContainer.test.tsx | 46 +++++++++++++++++++ .../ValidationAnimationContainer.tsx | 11 +++-- 3 files changed, 57 insertions(+), 5 deletions(-) create mode 100644 .changeset/validation-animation-derive-in-render.md create mode 100644 packages/react/src/internal/components/ValidationAnimationContainer.test.tsx diff --git a/.changeset/validation-animation-derive-in-render.md b/.changeset/validation-animation-derive-in-render.md new file mode 100644 index 00000000000..2061a816a89 --- /dev/null +++ b/.changeset/validation-animation-derive-in-render.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +FormControl: Show the validation message without an extra re-render by deriving its mounted state during render instead of in an effect. diff --git a/packages/react/src/internal/components/ValidationAnimationContainer.test.tsx b/packages/react/src/internal/components/ValidationAnimationContainer.test.tsx new file mode 100644 index 00000000000..b25e0e787ac --- /dev/null +++ b/packages/react/src/internal/components/ValidationAnimationContainer.test.tsx @@ -0,0 +1,46 @@ +import {render} from '@testing-library/react' +import {describe, it, expect} from 'vitest' +import ValidationAnimationContainer from './ValidationAnimationContainer' +import {createRenderCounter} from '../../utils/testing/profiler' + +describe('ValidationAnimationContainer', () => { + it('renders children when show is true', () => { + const {getByText} = render(content) + expect(getByText('content')).toBeInTheDocument() + }) + + it('does not render children when show is false initially', () => { + const {queryByText} = render(content) + expect(queryByText('content')).not.toBeInTheDocument() + }) + + it('renders children after show transitions to true', () => { + const {queryByText, rerender} = render( + content, + ) + expect(queryByText('content')).not.toBeInTheDocument() + + rerender(content) + expect(queryByText('content')).toBeInTheDocument() + }) + + it('commits once (no cascade) when show transitions to true', () => { + // The previous implementation set `shouldRender` from an effect, forcing an + // extra commit after the prop change. Deriving it during render keeps it to one. + const [Wrap, counter] = createRenderCounter() + const {rerender} = render( + + content + , + ) + counter.reset() + + rerender( + + content + , + ) + + expect(counter.updateCount).toBe(1) + }) +}) diff --git a/packages/react/src/internal/components/ValidationAnimationContainer.tsx b/packages/react/src/internal/components/ValidationAnimationContainer.tsx index 637502770f2..033ffd2c38f 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,11 @@ 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, 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]) + // Start rendering as soon as `show` becomes true. Adjusting state during render + // (instead of from an effect) avoids the extra post-commit render the effect caused. + if (show && !shouldRender) { + setRender(true) + } const onAnimationEnd = () => { if (!show) setRender(false) From afc0300d61806adbacd036cbb9132c64806b1fce Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Mon, 22 Jun 2026 15:33:37 +0000 Subject: [PATCH 2/3] test: exclude internal ValidationAnimationContainer from className-coverage check The new ValidationAnimationContainer render-count test triggers the classname-coverage check, but the internal animation wrapper does not forward a className prop, so implementsClassName does not apply. Add it to the ignore list. --- script/check-classname-tests.mjs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/script/check-classname-tests.mjs b/script/check-classname-tests.mjs index dd756028ec7..31086118605 100755 --- a/script/check-classname-tests.mjs +++ b/script/check-classname-tests.mjs @@ -20,6 +20,8 @@ const IGNORED_FILES = [ 'packages/react/src/FeatureFlags/__tests__/FeatureFlags.test.tsx', 'packages/react/src/FormControl/__tests__/useFormControlForwardedProps.test.tsx', 'packages/react/src/experimental/SelectPanel2/__tests__/SelectPanelLoading.test.tsx', + // Internal animation wrapper that does not forward a className prop + 'packages/react/src/internal/components/ValidationAnimationContainer.test.tsx', 'packages/react/src/__tests__/ThemeProvider.test.tsx', 'packages/react/src/__tests__/deprecated/ActionMenu.test.tsx', 'packages/react/src/__tests__/Caret.test.tsx', From 1a50c22f322c0e23be6720280dc992bb4ac7adc6 Mon Sep 17 00:00:00 2001 From: Matthew Costabile Date: Wed, 24 Jun 2026 01:20:50 +0000 Subject: [PATCH 3/3] Forward props/className, tighten render test, terse changeset --- .changeset/validation-animation-derive-in-render.md | 2 +- .../internal/components/ValidationAnimationContainer.test.tsx | 4 ++++ .../src/internal/components/ValidationAnimationContainer.tsx | 4 ++-- script/check-classname-tests.mjs | 2 -- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/.changeset/validation-animation-derive-in-render.md b/.changeset/validation-animation-derive-in-render.md index 2061a816a89..c74bd167c4f 100644 --- a/.changeset/validation-animation-derive-in-render.md +++ b/.changeset/validation-animation-derive-in-render.md @@ -2,4 +2,4 @@ '@primer/react': patch --- -FormControl: Show the validation message without an extra re-render by deriving its mounted state during render instead of in an effect. +FormControl: Avoid an extra re-render when showing a validation message. diff --git a/packages/react/src/internal/components/ValidationAnimationContainer.test.tsx b/packages/react/src/internal/components/ValidationAnimationContainer.test.tsx index b25e0e787ac..bb7f2cec9d9 100644 --- a/packages/react/src/internal/components/ValidationAnimationContainer.test.tsx +++ b/packages/react/src/internal/components/ValidationAnimationContainer.test.tsx @@ -2,8 +2,11 @@ import {render} from '@testing-library/react' import {describe, it, expect} from 'vitest' import ValidationAnimationContainer from './ValidationAnimationContainer' import {createRenderCounter} from '../../utils/testing/profiler' +import {implementsClassName} from '../../utils/testing' describe('ValidationAnimationContainer', () => { + implementsClassName(props => ) + it('renders children when show is true', () => { const {getByText} = render(content) expect(getByText('content')).toBeInTheDocument() @@ -42,5 +45,6 @@ describe('ValidationAnimationContainer', () => { ) expect(counter.updateCount).toBe(1) + expect(counter.nestedUpdateCount).toBe(0) }) }) diff --git a/packages/react/src/internal/components/ValidationAnimationContainer.tsx b/packages/react/src/internal/components/ValidationAnimationContainer.tsx index 033ffd2c38f..884ee15d2a7 100644 --- a/packages/react/src/internal/components/ValidationAnimationContainer.tsx +++ b/packages/react/src/internal/components/ValidationAnimationContainer.tsx @@ -6,7 +6,7 @@ import classes from './ValidationAnimationContainer.module.css' interface Props extends HTMLProps { show?: boolean } -const ValidationAnimationContainer: React.FC> = ({show, children}) => { +const ValidationAnimationContainer: React.FC> = ({show, children, style, ...rest}) => { const [shouldRender, setRender] = useState(show) // Start rendering as soon as `show` becomes true. Adjusting state during render @@ -20,7 +20,7 @@ const ValidationAnimationContainer: React.FC> = ( } return shouldRender ? ( -
+
{children}
diff --git a/script/check-classname-tests.mjs b/script/check-classname-tests.mjs index 31086118605..dd756028ec7 100755 --- a/script/check-classname-tests.mjs +++ b/script/check-classname-tests.mjs @@ -20,8 +20,6 @@ const IGNORED_FILES = [ 'packages/react/src/FeatureFlags/__tests__/FeatureFlags.test.tsx', 'packages/react/src/FormControl/__tests__/useFormControlForwardedProps.test.tsx', 'packages/react/src/experimental/SelectPanel2/__tests__/SelectPanelLoading.test.tsx', - // Internal animation wrapper that does not forward a className prop - 'packages/react/src/internal/components/ValidationAnimationContainer.test.tsx', 'packages/react/src/__tests__/ThemeProvider.test.tsx', 'packages/react/src/__tests__/deprecated/ActionMenu.test.tsx', 'packages/react/src/__tests__/Caret.test.tsx',