fix(useControlledState): apply function updates in order when uncontrolled

.changeset/fix-controlled-state-function-updates.md

@@ -0,0 +1,9 @@
+---
+'react-simplikit': patch
+---
+
+`useControlledState` now applies multiple `setValue` calls in the same tick in order when uncontrolled, instead of computing each from the value captured at render. This affects both function updates and plain values: `setValue(prev => prev + 3)` twice now adds 6 instead of 3, and `setValue('b')` followed by `setValue('a')` from a current value of `'a'` now settles on `'a'` instead of `'b'`.
+
+As a consequence, in uncontrolled mode `onChange` is called once after the state commits with the final value, rather than synchronously on every `setValue` call. A parent that mirrors `onChange` into its own state therefore renders once more per update, no call is made when the final value equals the previous one, and a change reported by a component that unmounts in the same commit is dropped. Under StrictMode a single change is now reported once instead of twice.
+
+Controlled mode is unchanged: it still computes from the current `value` prop, so two function updates in the same tick see the same previous value.

packages/react-simplikit/src/hooks/useControlledState/useControlledState.spec.tsx

@@ -1,6 +1,6 @@
-import { useState } from 'react';
-import { act, render, screen } from '@testing-library/react';
-import { describe, expect, it } from 'vitest';
+import { useLayoutEffect, useState } from 'react';
+import { act, fireEvent, render, screen } from '@testing-library/react';
+import { describe, expect, it, vi } from 'vitest';
 
 import { renderHookSSR } from '../../_internal/test-utils/renderHookSSR.tsx';
 
@@ -115,4 +115,241 @@ describe('useControlledState', () => {
     const [nextValue] = result.current;
     expect(nextValue).toBe(8);
   });
+
+  it('applies multiple function setState actions in order when uncontrolled', async () => {
+    const onChange = vi.fn();
+    const { result } = renderHookSSR(() => useControlledState({ defaultValue: 5, onChange }));
+
+    await act(async () => {
+      const [, setValue] = result.current;
+      setValue(prev => prev + 3);
+      setValue(prev => prev + 3);
+    });
+
+    const [nextValue] = result.current;
+    expect(nextValue).toBe(11);
+    expect(onChange).toHaveBeenCalledTimes(1);
+    expect(onChange).toHaveBeenCalledWith(11);
+  });
+
+  it('does not call onChange on mount when uncontrolled', () => {
+    const onChange = vi.fn();
+    renderHookSSR(() => useControlledState({ defaultValue: 5, onChange }));
+
+    expect(onChange).not.toHaveBeenCalled();
+  });
+
+  it('does not call onChange for a value equalityFn treats as equal when uncontrolled', async () => {
+    const onChange = vi.fn();
+    const { result } = renderHookSSR(() =>
+      useControlledState({
+        defaultValue: { id: 1 },
+        onChange,
+        equalityFn: (prev, next) => prev.id === next.id,
+      })
+    );
+
+    const [initialValue] = result.current;
+
+    await act(async () => {
+      const [, setValue] = result.current;
+      setValue({ id: 1 });
+    });
+
+    expect(onChange).not.toHaveBeenCalled();
+    expect(result.current[0]).toBe(initialValue);
+  });
+
+  it('reflects a value the parent changes externally when controlled', () => {
+    function App() {
+      const [checked, setChecked] = useState(true);
+      const [value] = useControlledState({ value: checked, onChange: setChecked });
+
+      return (
+        <div>
+          <p data-testid="value">{String(value)}</p>
+          <button onClick={() => setChecked(false)}>Clear value</button>
+        </div>
+      );
+    }
+
+    render(<App />);
+    expect(screen.getByTestId('value')).toHaveTextContent('true');
+
+    fireEvent.click(screen.getByText('Clear value'));
+    expect(screen.getByTestId('value')).toHaveTextContent('false');
+  });
+
+  it('toggles with a function setState action through the parent when controlled', () => {
+    const onChange = vi.fn();
+    function App() {
+      const [checked, setChecked] = useState(false);
+      const [value, setValue] = useControlledState({
+        value: checked,
+        onChange: next => {
+          onChange(next);
+          setChecked(next);
+        },
+      });
+
+      return <button role="checkbox" aria-checked={value} onClick={() => setValue(prev => !prev)} />;
+    }
+
+    render(<App />);
+    fireEvent.click(screen.getByRole('checkbox'));
+
+    expect(screen.getByRole('checkbox')).toHaveAttribute('aria-checked', 'true');
+    expect(onChange).toHaveBeenCalledTimes(1);
+    expect(onChange).toHaveBeenCalledWith(true);
+  });
+
+  it('does not re-render after the parent rejects a change when controlled', () => {
+    let renderCount = 0;
+    function App() {
+      renderCount += 1;
+      const [value, setValue] = useState(10);
+      const [state, setState] = useControlledState({
+        value,
+        onChange: next => setValue(Math.min(next, 10)),
+      });
+
+      useLayoutEffect(function pushEveryRender() {
+        setState(12);
+      });
+
+      return <p data-testid="value">{state}</p>;
+    }
+
+    render(<App />);
+
+    expect(screen.getByTestId('value')).toHaveTextContent('10');
+    expect(renderCount).toBe(1);
+  });
+
+  it('calls onChange once when setValue(undefined) switches from controlled back to uncontrolled', () => {
+    const onChange = vi.fn();
+    function App() {
+      const [prop, setProp] = useState<string | undefined>(undefined);
+      const [value, setValue] = useControlledState<string | undefined>({
+        value: prop,
+        defaultValue: 'a',
+        onChange: next => {
+          onChange(next);
+          setProp(next);
+        },
+      });
+
+      return (
+        <div>
+          <p data-testid="value">{String(value)}</p>
+          <button data-testid="control" onClick={() => setProp('b')} />
+          <button data-testid="clear" onClick={() => setValue(undefined)} />
+        </div>
+      );
+    }
+
+    render(<App />);
+    fireEvent.click(screen.getByTestId('control'));
+    expect(screen.getByTestId('value')).toHaveTextContent('b');
+
+    fireEvent.click(screen.getByTestId('clear'));
+    expect(screen.getByTestId('value')).toHaveTextContent('undefined');
+    expect(onChange).toHaveBeenCalledTimes(1);
+    expect(onChange).toHaveBeenCalledWith(undefined);
+  });
+
+  it('does not call onChange on mount when equalityFn is not reflexive when uncontrolled', () => {
+    const onChange = vi.fn();
+    renderHookSSR(() => useControlledState({ defaultValue: 5, onChange, equalityFn: () => false }));
+
+    expect(onChange).not.toHaveBeenCalled();
+  });
+
+  it('notifies through the latest onChange after the parent swaps it when uncontrolled', async () => {
+    const first = vi.fn();
+    const second = vi.fn();
+    const { result, rerender } = renderHookSSR(
+      ({ onChange }: { onChange: (next: number) => void }) => useControlledState({ defaultValue: 5, onChange }),
+      { initialProps: { onChange: first } }
+    );
+
+    rerender({ onChange: second });
+    await act(async () => {
+      const [, setValue] = result.current;
+      setValue(6);
+    });
+
+    expect(first).not.toHaveBeenCalled();
+    expect(second).toHaveBeenCalledWith(6);
+  });
+
+  it('notifies a change back to the initial value when uncontrolled', async () => {
+    const onChange = vi.fn();
+    const { result } = renderHookSSR(() => useControlledState({ defaultValue: 5, onChange }));
+
+    await act(async () => {
+      const [, setValue] = result.current;
+      setValue(6);
+    });
+    await act(async () => {
+      const [, setValue] = result.current;
+      setValue(5);
+    });
+
+    expect(onChange.mock.calls).toEqual([[6], [5]]);
+  });
+
+  it('notifies a change made in the same event the parent takes control', () => {
+    const onChange = vi.fn();
+    function App() {
+      const [prop, setProp] = useState<string | undefined>(undefined);
+      const [value, setValue] = useControlledState<string | undefined>({ value: prop, defaultValue: 'a', onChange });
+
+      return (
+        <div>
+          <p data-testid="value">{String(value)}</p>
+          <button
+            data-testid="take"
+            onClick={() => {
+              setValue('b');
+              setProp('b');
+            }}
+          />
+          <button data-testid="release" onClick={() => setProp(undefined)} />
+        </div>
+      );
+    }
+
+    render(<App />);
+    fireEvent.click(screen.getByTestId('take'));
+    expect(screen.getByTestId('value')).toHaveTextContent('b');
+    expect(onChange).toHaveBeenCalledTimes(1);
+    expect(onChange).toHaveBeenCalledWith('b');
+
+    fireEvent.click(screen.getByTestId('release'));
+    expect(onChange).toHaveBeenCalledTimes(1);
+  });
+
+  it('computes a function update from the value prop, not the internal state, when controlled', () => {
+    const onChange = vi.fn();
+    function App() {
+      const [count, setCount] = useState(5);
+      const [value, setValue] = useControlledState({
+        value: count,
+        defaultValue: 0,
+        onChange: next => {
+          onChange(next);
+          setCount(next);
+        },
+      });
+
+      return <button onClick={() => setValue(prev => prev + 3)}>{value}</button>;
+    }
+
+    render(<App />);
+    fireEvent.click(screen.getByRole('button'));
+
+    expect(screen.getByRole('button')).toHaveTextContent('8');
+    expect(onChange).toHaveBeenCalledWith(8);
+  });
 });

packages/react-simplikit/src/hooks/useControlledState/useControlledState.ts

@@ -1,4 +1,6 @@
-import { type Dispatch, type SetStateAction, useCallback, useState } from 'react';
+import { type Dispatch, type SetStateAction, useCallback, useEffect, useRef, useState } from 'react';
+
+import { usePreservedCallback } from '../usePreservedCallback/index.ts';
 
 type ControlledState<T> = { value: T; defaultValue?: never } | { defaultValue: T; value?: T };
 
@@ -51,14 +53,42 @@ export function useControlledState<T>({
   const [uncontrolledState, setUncontrolledState] = useState(defaultValue as T);
   const controlled = valueProp !== undefined;
   const value = controlled ? valueProp : uncontrolledState;
+  const preservedOnChange = usePreservedCallback((next: T) => onChange?.(next));
+
+  // Uncontrolled updates go through React's queue so function updates apply in order.
+  // onChange fires here, after commit, because the updater must stay pure (StrictMode runs it twice).
+  const prevUncontrolledRef = useRef(uncontrolledState);
+  useEffect(
+    function notifyUncontrolledChange() {
+      // The updater below already folds equal values into `prev`, so a reference check is enough here.
+      if (prevUncontrolledRef.current === uncontrolledState) return;
+      prevUncontrolledRef.current = uncontrolledState;
+      preservedOnChange(uncontrolledState);
+    },
+    [uncontrolledState, preservedOnChange]
+  );
 
   const setValue = useCallback(
     (next: SetStateAction<T>) => {
+      if (controlled === false) {
+        setUncontrolledState(prev => {
+          const nextValue = isSetStateAction(next) ? next(prev) : next;
+          return equalityFn(prev, nextValue) === true ? prev : nextValue;
+        });
+        return;
+      }
+
+      // Computed from the committed `value` on purpose: two function updates in the same tick
+      // see the same `prev`. Tracking the pending value in a ref needs a forced re-render to
+      // reset it, which loops when the parent rejects a change the caller re-issues every render.
       const nextValue = isSetStateAction(next) ? next(value) : next;
 
       if (equalityFn(value, nextValue) === true) return;
-      if (controlled === false) setUncontrolledState(nextValue);
-      if (controlled === true && nextValue === undefined) setUncontrolledState(nextValue);
+      if (nextValue === undefined) {
+        // Keep the notify effect from reporting this again once the parent hands control back.
+        prevUncontrolledRef.current = nextValue;
+        setUncontrolledState(nextValue);
+      }
       onChange?.(nextValue);
     },
     [controlled, onChange, equalityFn, value]