From 41ddb07edf101f8ccfd4cddb59b590aa54eb4e20 Mon Sep 17 00:00:00 2001 From: Bogdan Nikolic Date: Thu, 11 Jun 2026 15:48:01 +0200 Subject: [PATCH 1/9] ui: Stepper warns about duplicate step values at registration time The duplicate-value warning in Stepper.Root was dead code: registration de-duped steps before the warning loop could ever see a duplicate. Move the warning into useStepRegistration where it can actually fire, and return a no-op deregister for rejected duplicates so unmounting one no longer removes the original step registration. Follow-up to #111036 review feedback. Co-Authored-By: Claude Opus 4.6 --- packages/ui/src/stepper/root.tsx | 12 +--- packages/ui/src/stepper/test/index.tsx | 61 +++++++++++++++++-- .../ui/src/stepper/use-step-registration.ts | 24 +++++--- 3 files changed, 73 insertions(+), 24 deletions(-) diff --git a/packages/ui/src/stepper/root.tsx b/packages/ui/src/stepper/root.tsx index 787a60a32bfe..cf7f8c976179 100644 --- a/packages/ui/src/stepper/root.tsx +++ b/packages/ui/src/stepper/root.tsx @@ -72,22 +72,14 @@ export const StepperRoot = forwardRef< StepperRef, StepperRootProps >( function }, } ) ); - // Dev warnings: missing accessibility label, duplicate values, missing step + // Dev warnings: missing accessibility label, missing step. + // Duplicate step values are detected at registration time in useStepRegistration. useEffect( () => { if ( ! ariaLabel && ! ariaLabelledBy ) { warning( "[Stepper] Stepper requires either 'aria-label' or 'aria-labelledby' for accessibility." ); } - const seenValues = new Set< string >(); - for ( const s of steps ) { - if ( seenValues.has( s.value ) ) { - warning( - `[Stepper] Two steps share value '${ s.value }'. Each step must have a unique value.` - ); - } - seenValues.add( s.value ); - } if ( value && steps.length > 0 && ! steps.some( ( s ) => s.value === value ) ) { warning( `[Stepper] No step found with value '${ value }'. Check that the value matches one of the registered step values.` diff --git a/packages/ui/src/stepper/test/index.tsx b/packages/ui/src/stepper/test/index.tsx index 146f8dfe5929..5c9e371ac11c 100644 --- a/packages/ui/src/stepper/test/index.tsx +++ b/packages/ui/src/stepper/test/index.tsx @@ -592,12 +592,61 @@ describe( 'Stepper dev warnings', () => { spy.mockRestore(); } ); - // NOTE: A test for the duplicate-value warning ("Two steps share value '...'") is - // deliberately omitted. useStepRegistration prevents duplicate entries at registration - // time (it bails out if a step with the same value is already in state), so the - // steps array that the useEffect in root.tsx inspects will never contain duplicates. - // The warning is unreachable in a JSDOM environment via normal JSX rendering, making - // any such test a never-fails assertion that proves nothing about real behaviour. + // NOTE: warning() dedupes messages process-wide, so these tests use step values + // unique to this file to make sure the expected message has not fired elsewhere. + it( 'warns when two steps share the same value', async () => { + const spy = jest.spyOn( console, 'warn' ).mockImplementation( () => {} ); + render( + + + + + + + + + ); + await waitFor( () => { + expect( spy ).toHaveBeenCalledWith( + expect.stringContaining( "Two steps share value 'dup-warn'" ) + ); + } ); + spy.mockRestore(); + } ); + + it( 'keeps the original step registered when a duplicate step unmounts', async () => { + const spy = jest.spyOn( console, 'warn' ).mockImplementation( () => {} ); + function Dynamic() { + const [ showDuplicate, setShowDuplicate ] = useState( true ); + return ( + <> + + + + + + { showDuplicate && ( + + + + ) } + + + + + + ); + } + const user = userEvent.setup(); + render( ); + // The duplicate is rejected at registration time, so only two steps count. + await waitFor( () => expect( screen.getByText( 'Step 2 of 2' ) ).toBeInTheDocument() ); + await user.click( screen.getByRole( 'button', { name: /remove duplicate/i } ) ); + // Unmounting the rejected duplicate must not deregister the original step. + await waitFor( () => expect( screen.getByText( 'Step 2 of 2' ) ).toBeInTheDocument() ); + expect( screen.getByText( 'Step 1 of 2' ) ).toBeInTheDocument(); + spy.mockRestore(); + } ); } ); describe( 'Stepper.Panel dev warnings', () => { diff --git a/packages/ui/src/stepper/use-step-registration.ts b/packages/ui/src/stepper/use-step-registration.ts index 27466c68a3ac..80402c802f4d 100644 --- a/packages/ui/src/stepper/use-step-registration.ts +++ b/packages/ui/src/stepper/use-step-registration.ts @@ -1,4 +1,5 @@ -import { useCallback, useState } from '@wordpress/element'; +import { useCallback, useRef, useState } from '@wordpress/element'; +import { warning } from '../utils/warning'; // --------------------------------------------------------------------------- // Helpers @@ -14,16 +15,23 @@ import { useCallback, useState } from '@wordpress/element'; export function useStepRegistration< T extends { value: string } >() { const [ steps, setSteps ] = useState< T[] >( [] ); + // Tracks accepted registrations so duplicates can be rejected (with a dev + // warning) without putting side effects inside the state updater. + const registeredValues = useRef( new Set< string >() ); + const registerStep = useCallback( ( meta: T ) => { - setSteps( ( prev ) => { - // Avoid duplicate registration (e.g. from concurrent remounts or late renders) - if ( prev.some( ( s ) => s.value === meta.value ) ) { - return prev; - } - return [ ...prev, meta ]; - } ); + if ( registeredValues.current.has( meta.value ) ) { + warning( + `[Stepper] Two steps share value '${ meta.value }'. Each step must have a unique value.` + ); + // Rejected duplicate: deregistering it must not affect the original. + return () => {}; + } + registeredValues.current.add( meta.value ); + setSteps( ( prev ) => [ ...prev, meta ] ); return () => { + registeredValues.current.delete( meta.value ); setSteps( ( prev ) => prev.filter( ( s ) => s.value !== meta.value ) ); }; }, [] ); From a38140bd6f672d1b96621029649a3d2ef978e480 Mon Sep 17 00:00:00 2001 From: Bogdan Nikolic Date: Thu, 11 Jun 2026 15:49:55 +0200 Subject: [PATCH 2/9] ui: Add changelog entry for Stepper duplicate-value warning fix Co-Authored-By: Claude Opus 4.6 --- packages/ui/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/ui/CHANGELOG.md b/packages/ui/CHANGELOG.md index 1b8e94eb8da9..15be38254e3a 100644 --- a/packages/ui/CHANGELOG.md +++ b/packages/ui/CHANGELOG.md @@ -8,6 +8,10 @@ - `Stepper`, `VerticalStepper`, `HorizontalStepper`: Add Stepper component suite with vertical and horizontal orientations, step indicators, linear flow support, and accessible ARIA semantics ([#111036](https://github.com/Automattic/wp-calypso/pull/111036)). +### Bug Fixes + +- `Stepper`: Warn about duplicate step values at registration time (the previous check was unreachable), and keep the original step registered when a rejected duplicate unmounts ([#111567](https://github.com/Automattic/wp-calypso/pull/111567)). + ## 1.0.2 ### Enhancements From 02bc01cbbc4087d190a1a55631faf866554e81cf Mon Sep 17 00:00:00 2001 From: Bogdan Nikolic Date: Thu, 11 Jun 2026 22:56:59 +0200 Subject: [PATCH 3/9] ui: Drop changelog entry for unreleased Stepper fix The Stepper component is still unreleased, so a bug fix to it needs no changelog entry. Co-Authored-By: Claude Opus 4.6 --- packages/ui/CHANGELOG.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/ui/CHANGELOG.md b/packages/ui/CHANGELOG.md index 15be38254e3a..1b8e94eb8da9 100644 --- a/packages/ui/CHANGELOG.md +++ b/packages/ui/CHANGELOG.md @@ -8,10 +8,6 @@ - `Stepper`, `VerticalStepper`, `HorizontalStepper`: Add Stepper component suite with vertical and horizontal orientations, step indicators, linear flow support, and accessible ARIA semantics ([#111036](https://github.com/Automattic/wp-calypso/pull/111036)). -### Bug Fixes - -- `Stepper`: Warn about duplicate step values at registration time (the previous check was unreachable), and keep the original step registered when a rejected duplicate unmounts ([#111567](https://github.com/Automattic/wp-calypso/pull/111567)). - ## 1.0.2 ### Enhancements From cd69bb4d7f95a95b8feff9304c10d5a4917c48d0 Mon Sep 17 00:00:00 2001 From: Bogdan Nikolic Date: Thu, 11 Jun 2026 23:42:59 +0200 Subject: [PATCH 4/9] ui: Ref-count Stepper registrations so transient duplicates never drop a step A rejected duplicate previously got a no-op deregister, so if the originally-accepted instance unmounted while the duplicate stayed mounted, the value lost its registration entirely. Track a per-value mount count and remove the entry only when the last mounted instance deregisters. Co-Authored-By: Claude Opus 4.6 --- packages/ui/src/stepper/test/index.tsx | 34 +++++++++++++++++++ .../ui/src/stepper/use-step-registration.ts | 28 ++++++++++----- 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/packages/ui/src/stepper/test/index.tsx b/packages/ui/src/stepper/test/index.tsx index 5c9e371ac11c..1c2747b4faed 100644 --- a/packages/ui/src/stepper/test/index.tsx +++ b/packages/ui/src/stepper/test/index.tsx @@ -647,6 +647,40 @@ describe( 'Stepper dev warnings', () => { expect( screen.getByText( 'Step 1 of 2' ) ).toBeInTheDocument(); spy.mockRestore(); } ); + + it( 'keeps the step registered when the original unmounts but a duplicate is still mounted', async () => { + const spy = jest.spyOn( console, 'warn' ).mockImplementation( () => {} ); + function Dynamic() { + const [ showOriginal, setShowOriginal ] = useState( true ); + return ( + <> + + + { showOriginal && ( + + + + ) } + + + + + + + + + ); + } + const user = userEvent.setup(); + render( ); + // The duplicate is rejected at registration time, so only two steps count. + await waitFor( () => expect( screen.getByText( 'Step 2 of 2' ) ).toBeInTheDocument() ); + await user.click( screen.getByRole( 'button', { name: /remove original/i } ) ); + // The duplicate instance is still mounted, so the value must stay registered. + await waitFor( () => expect( screen.getByText( 'Step 2 of 2' ) ).toBeInTheDocument() ); + expect( screen.getByText( 'Step 1 of 2' ) ).toBeInTheDocument(); + spy.mockRestore(); + } ); } ); describe( 'Stepper.Panel dev warnings', () => { diff --git a/packages/ui/src/stepper/use-step-registration.ts b/packages/ui/src/stepper/use-step-registration.ts index 80402c802f4d..cb025c155c8f 100644 --- a/packages/ui/src/stepper/use-step-registration.ts +++ b/packages/ui/src/stepper/use-step-registration.ts @@ -15,23 +15,33 @@ import { warning } from '../utils/warning'; export function useStepRegistration< T extends { value: string } >() { const [ steps, setSteps ] = useState< T[] >( [] ); - // Tracks accepted registrations so duplicates can be rejected (with a dev - // warning) without putting side effects inside the state updater. - const registeredValues = useRef( new Set< string >() ); + // Per-value mount count. Only the first registration adds a state entry + // (duplicates get a dev warning), and the entry is removed only when the + // last mounted instance deregisters — so transient duplicates (e.g. + // overlapping conditional renders) never drop a still-mounted step. The + // ref also keeps the duplicate check and warning outside the state + // updater, which must stay pure. + const mountCounts = useRef( new Map< string, number >() ); const registerStep = useCallback( ( meta: T ) => { - if ( registeredValues.current.has( meta.value ) ) { + const counts = mountCounts.current; + const count = counts.get( meta.value ) ?? 0; + counts.set( meta.value, count + 1 ); + if ( count > 0 ) { warning( `[Stepper] Two steps share value '${ meta.value }'. Each step must have a unique value.` ); - // Rejected duplicate: deregistering it must not affect the original. - return () => {}; + } else { + setSteps( ( prev ) => [ ...prev, meta ] ); } - registeredValues.current.add( meta.value ); - setSteps( ( prev ) => [ ...prev, meta ] ); return () => { - registeredValues.current.delete( meta.value ); + const remaining = ( counts.get( meta.value ) ?? 1 ) - 1; + if ( remaining > 0 ) { + counts.set( meta.value, remaining ); + return; + } + counts.delete( meta.value ); setSteps( ( prev ) => prev.filter( ( s ) => s.value !== meta.value ) ); }; }, [] ); From 4dababd3c2d8bf9cec49284c65273e72076d78c6 Mon Sep 17 00:00:00 2001 From: Bogdan Nikolic Date: Fri, 12 Jun 2026 12:08:46 +0200 Subject: [PATCH 5/9] ui: Simplify Stepper updateStep compare and StepperPanelProps The union-keys comparison in updateStep was needed when registration meta carried ReactNode snapshots; meta is now flat scalars, so a plain shallow compare (with a key-count check for removed fields) is enough. Also drop children/className from StepperPanelProps since ComponentProps<'div'> already provides them. Follow-up to #111036 review feedback. Co-Authored-By: Claude Opus 4.6 --- .../stepper/test/use-step-registration.tsx | 63 +++++++++++++++++++ packages/ui/src/stepper/types.ts | 2 - .../ui/src/stepper/use-step-registration.ts | 13 ++-- 3 files changed, 70 insertions(+), 8 deletions(-) create mode 100644 packages/ui/src/stepper/test/use-step-registration.tsx diff --git a/packages/ui/src/stepper/test/use-step-registration.tsx b/packages/ui/src/stepper/test/use-step-registration.tsx new file mode 100644 index 000000000000..8c1f21e7cc5d --- /dev/null +++ b/packages/ui/src/stepper/test/use-step-registration.tsx @@ -0,0 +1,63 @@ +import { renderHook, act } from '@testing-library/react'; +import { useStepRegistration } from '../use-step-registration'; +import type { StepMeta } from '../types'; + +describe( 'useStepRegistration updateStep', () => { + function setup() { + const hook = renderHook( () => useStepRegistration< StepMeta >() ); + act( () => { + hook.result.current.registerStep( { value: 'a', status: 'completed', disabled: false } ); + hook.result.current.registerStep( { value: 'b', disabled: false } ); + } ); + return hook; + } + + it( 'keeps the same steps array reference when meta has not changed', () => { + const { result } = setup(); + const before = result.current.steps; + act( () => { + result.current.updateStep( { value: 'a', status: 'completed', disabled: false } ); + } ); + expect( result.current.steps ).toBe( before ); + } ); + + it( 'updates the entry when a field value changes', () => { + const { result } = setup(); + act( () => { + result.current.updateStep( { value: 'a', status: 'error', disabled: false } ); + } ); + expect( result.current.steps[ 0 ] ).toEqual( { + value: 'a', + status: 'error', + disabled: false, + } ); + } ); + + it( 'updates the entry when a field present in the existing meta is absent from the new meta', () => { + const { result } = setup(); + // `status` key intentionally omitted — the compare must not treat a + // removed field as "no change". + act( () => { + result.current.updateStep( { value: 'a', disabled: false } ); + } ); + expect( result.current.steps[ 0 ] ).toEqual( { value: 'a', disabled: false } ); + expect( result.current.steps[ 0 ] ).not.toHaveProperty( 'status' ); + } ); + + it( 'is a no-op for a value that was never registered', () => { + const { result } = setup(); + const before = result.current.steps; + act( () => { + result.current.updateStep( { value: 'missing', disabled: false } ); + } ); + expect( result.current.steps ).toBe( before ); + } ); + + it( 'preserves registration order when updating a step', () => { + const { result } = setup(); + act( () => { + result.current.updateStep( { value: 'a', status: 'error', disabled: true } ); + } ); + expect( result.current.steps.map( ( s ) => s.value ) ).toEqual( [ 'a', 'b' ] ); + } ); +} ); diff --git a/packages/ui/src/stepper/types.ts b/packages/ui/src/stepper/types.ts index 6750ef6aebb7..23625a922fec 100644 --- a/packages/ui/src/stepper/types.ts +++ b/packages/ui/src/stepper/types.ts @@ -154,6 +154,4 @@ export type StepperPanelProps = ComponentProps< 'div' > & { value?: string; /** Keep panel mounted when inactive (works in both orientations) */ keepMounted?: boolean; - children: ReactNode; - className?: string; }; diff --git a/packages/ui/src/stepper/use-step-registration.ts b/packages/ui/src/stepper/use-step-registration.ts index cb025c155c8f..c0bac16d6f0b 100644 --- a/packages/ui/src/stepper/use-step-registration.ts +++ b/packages/ui/src/stepper/use-step-registration.ts @@ -53,12 +53,13 @@ export function useStepRegistration< T extends { value: string } >() { return prev; } const existing = prev[ idx ]; - // Union keys from both objects so fields present in existing but - // absent from meta are not silently ignored. - const keys = [ - ...new Set( [ ...Object.keys( existing ), ...Object.keys( meta ) ] ), - ] as ( keyof T )[]; - if ( keys.every( ( k ) => existing[ k ] === meta[ k ] ) ) { + // Plain shallow compare. The key-count check catches fields present + // in existing but absent from meta (e.g. a removed `status`). + const metaKeys = Object.keys( meta ) as ( keyof T )[]; + const isUnchanged = + Object.keys( existing ).length === metaKeys.length && + metaKeys.every( ( k ) => existing[ k ] === meta[ k ] ); + if ( isUnchanged ) { return prev; } return prev.map( ( s ) => ( s.value === meta.value ? meta : s ) ); From b32b63f22676df53e54bebf657136b95df2eb280 Mon Sep 17 00:00:00 2001 From: Bogdan Nikolic Date: Mon, 15 Jun 2026 23:24:21 +0200 Subject: [PATCH 6/9] Address PR review feedback (#111567) - Document the stale-metadata limitation when duplicate step values overlap - Rename use-step-registration test to .ts (no JSX) Co-Authored-By: Claude Opus 4.7 --- ...-step-registration.tsx => use-step-registration.ts} | 0 packages/ui/src/stepper/use-step-registration.ts | 10 ++++++++++ 2 files changed, 10 insertions(+) rename packages/ui/src/stepper/test/{use-step-registration.tsx => use-step-registration.ts} (100%) diff --git a/packages/ui/src/stepper/test/use-step-registration.tsx b/packages/ui/src/stepper/test/use-step-registration.ts similarity index 100% rename from packages/ui/src/stepper/test/use-step-registration.tsx rename to packages/ui/src/stepper/test/use-step-registration.ts diff --git a/packages/ui/src/stepper/use-step-registration.ts b/packages/ui/src/stepper/use-step-registration.ts index c0bac16d6f0b..b05fb68e37d9 100644 --- a/packages/ui/src/stepper/use-step-registration.ts +++ b/packages/ui/src/stepper/use-step-registration.ts @@ -38,6 +38,16 @@ export function useStepRegistration< T extends { value: string } >() { return () => { const remaining = ( counts.get( meta.value ) ?? 1 ) - 1; if ( remaining > 0 ) { + // A duplicate instance is still mounted, so keep the value + // registered to protect index/totalSteps. Limitation: `steps` + // keeps this unmounted instance's metadata and the survivor + // never re-syncs (its updateStep effect only reacts to its own + // props), so `status`/`disabled` can render stale until the + // survivor's props next change. Acceptable since duplicate + // values are already a warned dev error. + // TODO: for correct metadata under duplicates, switch to + // instance-keyed registration (unique id per mount, derive + // `steps` from the first still-mounted instance per value). counts.set( meta.value, remaining ); return; } From 80cc0d3672773e7336aff5c52a4d26fe6025896e Mon Sep 17 00:00:00 2001 From: Bogdan Nikolic Date: Mon, 15 Jun 2026 23:47:42 +0200 Subject: [PATCH 7/9] ui: Key Stepper registration by per-mount id to fix stale duplicate metadata Instead of keying steps by value, each Stepper.Step now registers under a unique useId. The public steps list is derived as one entry per value from the first still-mounted instance, so a duplicate value can no longer override or leave stale metadata on the surviving step. Co-Authored-By: Claude Opus 4.7 --- packages/ui/src/stepper/step.tsx | 14 ++-- .../src/stepper/test/use-step-registration.ts | 75 +++++++++++++++++-- packages/ui/src/stepper/types.ts | 4 +- .../ui/src/stepper/use-step-registration.ts | 68 ++++++++++------- 4 files changed, 117 insertions(+), 44 deletions(-) diff --git a/packages/ui/src/stepper/step.tsx b/packages/ui/src/stepper/step.tsx index c56c53a45b01..12bac1371a1d 100644 --- a/packages/ui/src/stepper/step.tsx +++ b/packages/ui/src/stepper/step.tsx @@ -1,6 +1,6 @@ // packages/ui/src/stepper/step.tsx import { Accordion } from '@base-ui/react/accordion'; -import { forwardRef, useContext, useEffect, useLayoutEffect } from '@wordpress/element'; +import { forwardRef, useContext, useEffect, useId, useLayoutEffect } from '@wordpress/element'; import { StepContext, StepperContext } from './context'; import type { StepContextValue, StepStatus } from './types'; import type { ReactNode } from 'react'; @@ -33,6 +33,10 @@ export const StepperStep = forwardRef< HTMLDivElement, StepperStepProps >( funct orientation, } = rootCtx; + // Stable per-mount id so each instance owns its own registration, even when + // two steps transiently share the same `value`. + const id = useId(); + // Compute derived state const isCurrent = value === activeValue; const index = steps.findIndex( ( s ) => s.value === value ); @@ -47,17 +51,17 @@ export const StepperStep = forwardRef< HTMLDivElement, StepperStepProps >( funct // useLayoutEffect ensures registration happens before the first browser paint, // so totalSteps and index are correct on the first visible render. useLayoutEffect( () => { - const deregister = registerStep( { value, status, disabled: isDisabled } ); + const deregister = registerStep( id, { value, status, disabled: isDisabled } ); return deregister; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ value ] ); + }, [ id, value ] ); // useEffect (not useLayoutEffect) is intentional: syncing metadata after // commit does not affect the initial render. The redundant call on first // mount is harmless — updateStep is a no-op when props have not changed. useEffect( () => { - updateStep( { value, status, disabled: isDisabled } ); - }, [ value, status, isDisabled, updateStep ] ); + updateStep( id, { value, status, disabled: isDisabled } ); + }, [ id, value, status, isDisabled, updateStep ] ); const stepCtx: StepContextValue = { value, diff --git a/packages/ui/src/stepper/test/use-step-registration.ts b/packages/ui/src/stepper/test/use-step-registration.ts index 8c1f21e7cc5d..ae175f94d835 100644 --- a/packages/ui/src/stepper/test/use-step-registration.ts +++ b/packages/ui/src/stepper/test/use-step-registration.ts @@ -2,12 +2,71 @@ import { renderHook, act } from '@testing-library/react'; import { useStepRegistration } from '../use-step-registration'; import type { StepMeta } from '../types'; +describe( 'useStepRegistration duplicate values', () => { + let warnSpy: jest.SpyInstance; + + beforeEach( () => { + // Duplicate values are a dev error and emit a one-time console warning; + // suppress it here so test output stays clean. + warnSpy = jest.spyOn( console, 'warn' ).mockImplementation( () => {} ); + } ); + + afterEach( () => { + warnSpy.mockRestore(); + } ); + + it( 'reflects the surviving instance metadata when a duplicate unmounts', () => { + const { result } = renderHook( () => useStepRegistration< StepMeta >() ); + let deregisterDuplicate: () => void = () => {}; + act( () => { + result.current.registerStep( 'a-1', { value: 'a', status: 'completed', disabled: false } ); + deregisterDuplicate = result.current.registerStep( 'a-2', { + value: 'a', + status: 'error', + disabled: true, + } ); + } ); + // In real usage each Step's effect syncs its own props; the duplicate's + // sync would overwrite the shared-value entry under value-keyed storage. + act( () => { + result.current.updateStep( 'a-2', { value: 'a', status: 'error', disabled: true } ); + } ); + act( () => { + deregisterDuplicate(); + } ); + // The survivor is instance 'a-1'; its metadata must be reflected, not the + // unmounted duplicate's stale data. + expect( result.current.steps ).toEqual( [ + { value: 'a', status: 'completed', disabled: false }, + ] ); + } ); + + it( 'keeps the value registered until the last instance unmounts', () => { + const { result } = renderHook( () => useStepRegistration< StepMeta >() ); + let deregisterFirst: () => void = () => {}; + act( () => { + deregisterFirst = result.current.registerStep( 'a-1', { value: 'a', disabled: false } ); + result.current.registerStep( 'a-2', { value: 'a', disabled: false } ); + } ); + act( () => { + deregisterFirst(); + } ); + // One instance remains, so the value stays registered and reflects the + // surviving instance. + expect( result.current.steps ).toEqual( [ { value: 'a', disabled: false } ] ); + } ); +} ); + describe( 'useStepRegistration updateStep', () => { function setup() { const hook = renderHook( () => useStepRegistration< StepMeta >() ); act( () => { - hook.result.current.registerStep( { value: 'a', status: 'completed', disabled: false } ); - hook.result.current.registerStep( { value: 'b', disabled: false } ); + hook.result.current.registerStep( 'a', { + value: 'a', + status: 'completed', + disabled: false, + } ); + hook.result.current.registerStep( 'b', { value: 'b', disabled: false } ); } ); return hook; } @@ -16,7 +75,7 @@ describe( 'useStepRegistration updateStep', () => { const { result } = setup(); const before = result.current.steps; act( () => { - result.current.updateStep( { value: 'a', status: 'completed', disabled: false } ); + result.current.updateStep( 'a', { value: 'a', status: 'completed', disabled: false } ); } ); expect( result.current.steps ).toBe( before ); } ); @@ -24,7 +83,7 @@ describe( 'useStepRegistration updateStep', () => { it( 'updates the entry when a field value changes', () => { const { result } = setup(); act( () => { - result.current.updateStep( { value: 'a', status: 'error', disabled: false } ); + result.current.updateStep( 'a', { value: 'a', status: 'error', disabled: false } ); } ); expect( result.current.steps[ 0 ] ).toEqual( { value: 'a', @@ -38,17 +97,17 @@ describe( 'useStepRegistration updateStep', () => { // `status` key intentionally omitted — the compare must not treat a // removed field as "no change". act( () => { - result.current.updateStep( { value: 'a', disabled: false } ); + result.current.updateStep( 'a', { value: 'a', disabled: false } ); } ); expect( result.current.steps[ 0 ] ).toEqual( { value: 'a', disabled: false } ); expect( result.current.steps[ 0 ] ).not.toHaveProperty( 'status' ); } ); - it( 'is a no-op for a value that was never registered', () => { + it( 'is a no-op for an id that was never registered', () => { const { result } = setup(); const before = result.current.steps; act( () => { - result.current.updateStep( { value: 'missing', disabled: false } ); + result.current.updateStep( 'missing', { value: 'missing', disabled: false } ); } ); expect( result.current.steps ).toBe( before ); } ); @@ -56,7 +115,7 @@ describe( 'useStepRegistration updateStep', () => { it( 'preserves registration order when updating a step', () => { const { result } = setup(); act( () => { - result.current.updateStep( { value: 'a', status: 'error', disabled: true } ); + result.current.updateStep( 'a', { value: 'a', status: 'error', disabled: true } ); } ); expect( result.current.steps.map( ( s ) => s.value ) ).toEqual( [ 'a', 'b' ] ); } ); diff --git a/packages/ui/src/stepper/types.ts b/packages/ui/src/stepper/types.ts index 23625a922fec..b0d4f69e4e9d 100644 --- a/packages/ui/src/stepper/types.ts +++ b/packages/ui/src/stepper/types.ts @@ -47,8 +47,8 @@ export type StepperContextValue = { activationMode: 'auto' | 'manual'; steps: StepMeta[]; totalSteps: number; - registerStep: ( meta: StepMeta ) => () => void; - updateStep: ( meta: StepMeta ) => void; + registerStep: ( id: string, meta: StepMeta ) => () => void; + updateStep: ( id: string, meta: StepMeta ) => void; registerTriggerRef: ( value: string, el: HTMLElement | null ) => void; formatStepLabel: ( step: number, total: number, status?: StepStatus ) => string; indicatorVariant: IndicatorVariant; diff --git a/packages/ui/src/stepper/use-step-registration.ts b/packages/ui/src/stepper/use-step-registration.ts index b05fb68e37d9..82f87f7448af 100644 --- a/packages/ui/src/stepper/use-step-registration.ts +++ b/packages/ui/src/stepper/use-step-registration.ts @@ -1,29 +1,33 @@ -import { useCallback, useRef, useState } from '@wordpress/element'; +import { useCallback, useMemo, useRef, useState } from '@wordpress/element'; import { warning } from '../utils/warning'; // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- +type Instance< T > = { id: string; meta: T }; + /** * Maintains an ordered list of registered steps for any record type that * includes a `value` string. Registration order is the sole source of truth * for index and counting. * + * Registrations are keyed by a unique per-mount `id` (not `value`), so each + * mounted instance owns its own metadata. The public `steps` list is derived + * with one entry per `value`, taken from the first still-mounted instance — + * this means a duplicate value never overrides or leaves stale metadata on the + * surviving step. Duplicate values are still a dev error and emit a warning. + * * Used by Stepper.Root (with StepMeta) to maintain step order and metadata. */ export function useStepRegistration< T extends { value: string } >() { - const [ steps, setSteps ] = useState< T[] >( [] ); + const [ instances, setInstances ] = useState< Instance< T >[] >( [] ); - // Per-value mount count. Only the first registration adds a state entry - // (duplicates get a dev warning), and the entry is removed only when the - // last mounted instance deregisters — so transient duplicates (e.g. - // overlapping conditional renders) never drop a still-mounted step. The - // ref also keeps the duplicate check and warning outside the state - // updater, which must stay pure. + // Per-value mount count, used only to warn on duplicate values without + // reaching into the (pure) state updater. const mountCounts = useRef( new Map< string, number >() ); - const registerStep = useCallback( ( meta: T ) => { + const registerStep = useCallback( ( id: string, meta: T ) => { const counts = mountCounts.current; const count = counts.get( meta.value ) ?? 0; counts.set( meta.value, count + 1 ); @@ -31,38 +35,27 @@ export function useStepRegistration< T extends { value: string } >() { warning( `[Stepper] Two steps share value '${ meta.value }'. Each step must have a unique value.` ); - } else { - setSteps( ( prev ) => [ ...prev, meta ] ); } + setInstances( ( prev ) => [ ...prev, { id, meta } ] ); return () => { const remaining = ( counts.get( meta.value ) ?? 1 ) - 1; if ( remaining > 0 ) { - // A duplicate instance is still mounted, so keep the value - // registered to protect index/totalSteps. Limitation: `steps` - // keeps this unmounted instance's metadata and the survivor - // never re-syncs (its updateStep effect only reacts to its own - // props), so `status`/`disabled` can render stale until the - // survivor's props next change. Acceptable since duplicate - // values are already a warned dev error. - // TODO: for correct metadata under duplicates, switch to - // instance-keyed registration (unique id per mount, derive - // `steps` from the first still-mounted instance per value). counts.set( meta.value, remaining ); - return; + } else { + counts.delete( meta.value ); } - counts.delete( meta.value ); - setSteps( ( prev ) => prev.filter( ( s ) => s.value !== meta.value ) ); + setInstances( ( prev ) => prev.filter( ( inst ) => inst.id !== id ) ); }; }, [] ); - const updateStep = useCallback( ( meta: T ) => { - setSteps( ( prev ) => { - const idx = prev.findIndex( ( s ) => s.value === meta.value ); + const updateStep = useCallback( ( id: string, meta: T ) => { + setInstances( ( prev ) => { + const idx = prev.findIndex( ( inst ) => inst.id === id ); if ( idx === -1 ) { return prev; } - const existing = prev[ idx ]; + const existing = prev[ idx ].meta; // Plain shallow compare. The key-count check catches fields present // in existing but absent from meta (e.g. a removed `status`). const metaKeys = Object.keys( meta ) as ( keyof T )[]; @@ -72,9 +65,26 @@ export function useStepRegistration< T extends { value: string } >() { if ( isUnchanged ) { return prev; } - return prev.map( ( s ) => ( s.value === meta.value ? meta : s ) ); + return prev.map( ( inst ) => ( inst.id === id ? { id, meta } : inst ) ); } ); }, [] ); + // One step per value, taken from the first still-mounted instance and + // ordered by that value's first appearance in registration order. Memoised + // so a no-op update (which leaves `instances` referentially stable) keeps + // the same `steps` reference. + const steps = useMemo( () => { + const seen = new Set< string >(); + const result: T[] = []; + for ( const inst of instances ) { + if ( seen.has( inst.meta.value ) ) { + continue; + } + seen.add( inst.meta.value ); + result.push( inst.meta ); + } + return result; + }, [ instances ] ); + return { steps, registerStep, updateStep }; } From 4413e3f591c58d737bd38f8538cd817d8bbc62a5 Mon Sep 17 00:00:00 2001 From: Bogdan Nikolic Date: Tue, 16 Jun 2026 11:57:35 +0200 Subject: [PATCH 8/9] ui: Pin Stepper step slot so a surviving duplicate keeps its position Order derived steps by each value's stable first-appearance slot instead of its current array position, so when an earlier duplicate instance unmounts the surviving instance keeps the value's original slot rather than jumping to the end. Adds tests for the slot invariant, the duplicate warning, and strengthens the surviving-metadata assertion to cover both mounted and unmounted states. Co-Authored-By: Claude Opus 4.7 --- .../src/stepper/test/use-step-registration.ts | 64 ++++++++++++++++++- .../ui/src/stepper/use-step-registration.ts | 34 ++++++---- 2 files changed, 84 insertions(+), 14 deletions(-) diff --git a/packages/ui/src/stepper/test/use-step-registration.ts b/packages/ui/src/stepper/test/use-step-registration.ts index ae175f94d835..2e22bd688d6f 100644 --- a/packages/ui/src/stepper/test/use-step-registration.ts +++ b/packages/ui/src/stepper/test/use-step-registration.ts @@ -27,20 +27,44 @@ describe( 'useStepRegistration duplicate values', () => { } ); } ); // In real usage each Step's effect syncs its own props; the duplicate's - // sync would overwrite the shared-value entry under value-keyed storage. + // sync must not overwrite the shared-value entry (the value-keyed bug). act( () => { result.current.updateStep( 'a-2', { value: 'a', status: 'error', disabled: true } ); } ); + // Even while both are mounted, the canonical entry stays 'a-1', not the + // later writer 'a-2'. + expect( result.current.steps ).toEqual( [ + { value: 'a', status: 'completed', disabled: false }, + ] ); act( () => { deregisterDuplicate(); } ); - // The survivor is instance 'a-1'; its metadata must be reflected, not the - // unmounted duplicate's stale data. + // ...and it remains correct after the duplicate unmounts. expect( result.current.steps ).toEqual( [ { value: 'a', status: 'completed', disabled: false }, ] ); } ); + it( 'keeps a value in its original slot when an earlier duplicate unmounts', () => { + const { result } = renderHook( () => useStepRegistration< StepMeta >() ); + let deregisterFirstA: () => void = () => {}; + act( () => { + deregisterFirstA = result.current.registerStep( 'a-1', { value: 'a', disabled: false } ); + result.current.registerStep( 'b-1', { value: 'b', disabled: false } ); + // Duplicate of 'a', registered after 'b'. + result.current.registerStep( 'a-2', { value: 'a', disabled: true } ); + } ); + act( () => { + deregisterFirstA(); + } ); + // 'a' keeps its first-appearance slot (index 0) rather than jumping to the + // end, and now reflects the surviving instance ('a-2'). + expect( result.current.steps ).toEqual( [ + { value: 'a', disabled: true }, + { value: 'b', disabled: false }, + ] ); + } ); + it( 'keeps the value registered until the last instance unmounts', () => { const { result } = renderHook( () => useStepRegistration< StepMeta >() ); let deregisterFirst: () => void = () => {}; @@ -57,6 +81,40 @@ describe( 'useStepRegistration duplicate values', () => { } ); } ); +describe( 'useStepRegistration duplicate warning', () => { + let warnSpy: jest.SpyInstance; + + beforeEach( () => { + warnSpy = jest.spyOn( console, 'warn' ).mockImplementation( () => {} ); + } ); + + afterEach( () => { + warnSpy.mockRestore(); + } ); + + it( 'warns when a second instance registers an already-registered value', () => { + const { result } = renderHook( () => useStepRegistration< StepMeta >() ); + act( () => { + result.current.registerStep( 'dup-1', { value: 'shared', disabled: false } ); + result.current.registerStep( 'dup-2', { value: 'shared', disabled: false } ); + } ); + // Distinct values per test keep the warning util's message-level dedupe + // from masking this assertion across the file. + expect( warnSpy ).toHaveBeenCalledWith( + expect.stringContaining( "Two steps share value 'shared'" ) + ); + } ); + + it( 'does not warn when every step value is unique', () => { + const { result } = renderHook( () => useStepRegistration< StepMeta >() ); + act( () => { + result.current.registerStep( 'u-1', { value: 'one', disabled: false } ); + result.current.registerStep( 'u-2', { value: 'two', disabled: false } ); + } ); + expect( warnSpy ).not.toHaveBeenCalled(); + } ); +} ); + describe( 'useStepRegistration updateStep', () => { function setup() { const hook = renderHook( () => useStepRegistration< StepMeta >() ); diff --git a/packages/ui/src/stepper/use-step-registration.ts b/packages/ui/src/stepper/use-step-registration.ts index 82f87f7448af..229b261540d2 100644 --- a/packages/ui/src/stepper/use-step-registration.ts +++ b/packages/ui/src/stepper/use-step-registration.ts @@ -16,7 +16,10 @@ type Instance< T > = { id: string; meta: T }; * mounted instance owns its own metadata. The public `steps` list is derived * with one entry per `value`, taken from the first still-mounted instance — * this means a duplicate value never overrides or leaves stale metadata on the - * surviving step. Duplicate values are still a dev error and emit a warning. + * surviving step. A value holds the slot it was first registered in for as long + * as any instance of it stays mounted, so a surviving duplicate does not jump + * position when an earlier instance unmounts. Duplicate values are still a dev + * error and emit a warning. * * Used by Stepper.Root (with StepMeta) to maintain step order and metadata. */ @@ -27,6 +30,12 @@ export function useStepRegistration< T extends { value: string } >() { // reaching into the (pure) state updater. const mountCounts = useRef( new Map< string, number >() ); + // Stable slot per value, assigned when a value first mounts and held until + // its last instance unmounts. Keeps `steps` order pinned to first-appearance + // even as duplicate instances of a value come and go. + const slotOrder = useRef( new Map< string, number >() ); + const nextSlot = useRef( 0 ); + const registerStep = useCallback( ( id: string, meta: T ) => { const counts = mountCounts.current; const count = counts.get( meta.value ) ?? 0; @@ -35,6 +44,8 @@ export function useStepRegistration< T extends { value: string } >() { warning( `[Stepper] Two steps share value '${ meta.value }'. Each step must have a unique value.` ); + } else { + slotOrder.current.set( meta.value, nextSlot.current++ ); } setInstances( ( prev ) => [ ...prev, { id, meta } ] ); @@ -44,6 +55,7 @@ export function useStepRegistration< T extends { value: string } >() { counts.set( meta.value, remaining ); } else { counts.delete( meta.value ); + slotOrder.current.delete( meta.value ); } setInstances( ( prev ) => prev.filter( ( inst ) => inst.id !== id ) ); }; @@ -70,20 +82,20 @@ export function useStepRegistration< T extends { value: string } >() { }, [] ); // One step per value, taken from the first still-mounted instance and - // ordered by that value's first appearance in registration order. Memoised - // so a no-op update (which leaves `instances` referentially stable) keeps - // the same `steps` reference. + // ordered by each value's stable first-appearance slot. Memoised so a no-op + // update (which leaves `instances` referentially stable) keeps the same + // `steps` reference. const steps = useMemo( () => { - const seen = new Set< string >(); - const result: T[] = []; + const order = slotOrder.current; + const canonical = new Map< string, T >(); for ( const inst of instances ) { - if ( seen.has( inst.meta.value ) ) { - continue; + if ( ! canonical.has( inst.meta.value ) ) { + canonical.set( inst.meta.value, inst.meta ); } - seen.add( inst.meta.value ); - result.push( inst.meta ); } - return result; + return [ ...canonical.values() ].sort( + ( a, b ) => ( order.get( a.value ) ?? 0 ) - ( order.get( b.value ) ?? 0 ) + ); }, [ instances ] ); return { steps, registerStep, updateStep }; From 6eee6d3c0f86a6edbdb6214c1cc919d2ceb6e92a Mon Sep 17 00:00:00 2001 From: Bogdan Nikolic Date: Wed, 17 Jun 2026 00:00:58 +0200 Subject: [PATCH 9/9] ui: Store Stepper slot in state so steps derivation stays pure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fold the per-value slot into the instances state and sort on it, so the steps memo no longer reads the slotOrder ref mid-render — keeping the derivation pure under concurrent rendering. Co-Authored-By: Claude Opus 4.7 --- .../ui/src/stepper/use-step-registration.ts | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/packages/ui/src/stepper/use-step-registration.ts b/packages/ui/src/stepper/use-step-registration.ts index 229b261540d2..6cc1e099f579 100644 --- a/packages/ui/src/stepper/use-step-registration.ts +++ b/packages/ui/src/stepper/use-step-registration.ts @@ -5,7 +5,7 @@ import { warning } from '../utils/warning'; // Helpers // --------------------------------------------------------------------------- -type Instance< T > = { id: string; meta: T }; +type Instance< T > = { id: string; slot: number; meta: T }; /** * Maintains an ordered list of registered steps for any record type that @@ -31,8 +31,10 @@ export function useStepRegistration< T extends { value: string } >() { const mountCounts = useRef( new Map< string, number >() ); // Stable slot per value, assigned when a value first mounts and held until - // its last instance unmounts. Keeps `steps` order pinned to first-appearance - // even as duplicate instances of a value come and go. + // its last instance unmounts. Duplicate instances of a value reuse its slot, + // so each carries its slot in state and `steps` order stays pinned to + // first-appearance even as duplicates come and go. Read only here (never + // during render) so the derivation stays pure. const slotOrder = useRef( new Map< string, number >() ); const nextSlot = useRef( 0 ); @@ -44,10 +46,13 @@ export function useStepRegistration< T extends { value: string } >() { warning( `[Stepper] Two steps share value '${ meta.value }'. Each step must have a unique value.` ); - } else { - slotOrder.current.set( meta.value, nextSlot.current++ ); } - setInstances( ( prev ) => [ ...prev, { id, meta } ] ); + let slot = slotOrder.current.get( meta.value ); + if ( slot === undefined ) { + slot = nextSlot.current++; + slotOrder.current.set( meta.value, slot ); + } + setInstances( ( prev ) => [ ...prev, { id, slot, meta } ] ); return () => { const remaining = ( counts.get( meta.value ) ?? 1 ) - 1; @@ -77,7 +82,7 @@ export function useStepRegistration< T extends { value: string } >() { if ( isUnchanged ) { return prev; } - return prev.map( ( inst ) => ( inst.id === id ? { id, meta } : inst ) ); + return prev.map( ( inst ) => ( inst.id === id ? { ...inst, meta } : inst ) ); } ); }, [] ); @@ -86,16 +91,15 @@ export function useStepRegistration< T extends { value: string } >() { // update (which leaves `instances` referentially stable) keeps the same // `steps` reference. const steps = useMemo( () => { - const order = slotOrder.current; - const canonical = new Map< string, T >(); + const canonical = new Map< string, { slot: number; meta: T } >(); for ( const inst of instances ) { if ( ! canonical.has( inst.meta.value ) ) { - canonical.set( inst.meta.value, inst.meta ); + canonical.set( inst.meta.value, { slot: inst.slot, meta: inst.meta } ); } } - return [ ...canonical.values() ].sort( - ( a, b ) => ( order.get( a.value ) ?? 0 ) - ( order.get( b.value ) ?? 0 ) - ); + return [ ...canonical.values() ] + .sort( ( a, b ) => a.slot - b.slot ) + .map( ( entry ) => entry.meta ); }, [ instances ] ); return { steps, registerStep, updateStep };