Form renderer: a defaultValues reset is not a user edit for memoized callers either

.changeset/form-reset-notification-channels-5235.md

@@ -0,0 +1,28 @@
+---
+'@object-ui/components': patch
+---
+
+The form renderer now keeps a `defaultValues` reset off `onChange` and off the
+`form_change` `onAction` for **every** caller — including one that memoizes the
+callback (objectui#5235).
+
+"A record landing is not a user edit" was already this file's documented,
+pinned behaviour, but two of the three channels delivered it by accident of
+React's effect ordering: every layout DESTROY runs before any layout CREATE, so
+a caller passing a fresh callback each render had its value subscription torn
+down before the reset and re-established after. The guarantee was therefore
+delivered by the callback's *identity changing*. Wrap the same callback in
+`React.useCallback` — taught everywhere as a semantically neutral performance
+optimization — and the identity stays put, the effect never re-runs, the
+subscription survives the reset, and the whole loaded record comes back to the
+host as if the user had typed it: the false "the user edited this" signal
+objectui#2968 was filed about, in a form no type, doc or call site warned about.
+
+The reset now states what those two channels report, the way `onDirtyChange`
+already did (it computes its payload against the freshly installed baseline and
+calls the host outright). Callers passing inline arrows see byte-identical
+behaviour; callers who memoize stop receiving a phantom edit.
+
+Not a contract change: whether a value channel *should* report a programmatic
+reset stays open in objectui#5235. This only removes the answer's dependence on
+caller identity.

packages/components/src/renderers/form/__tests__/form-reset-notification-channels.test.tsx

@@ -0,0 +1,180 @@
+/**
+ * ObjectUI
+ * Copyright (c) 2024-present ObjectStack Inc.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+/**
+ * "A `defaultValues` reset is not a user edit" must hold for EVERY caller —
+ * memoized or not (#5235).
+ *
+ * The renderer publishes three notification channels around a reset, and until
+ * this card they were driven two different ways:
+ *
+ *   - `onDirtyChange` — driven EXPLICITLY: the reset effect computes dirtiness
+ *     against the baseline it just installed and calls the callback outright.
+ *     Identity-independent by construction.
+ *   - `onChange` and `onAction({ type: 'form_change' })` — rode on React's
+ *     effect ordering. Every layout DESTROY runs before any layout CREATE, so a
+ *     caller passing a FRESH callback each render had its subscription torn
+ *     down before the reset and re-established after. The guarantee was
+ *     delivered by the callback's IDENTITY CHANGING — so a caller who wrapped
+ *     the same callback in `React.useCallback` (taught everywhere as a pure
+ *     performance optimization) kept one identity, the effect never re-ran, the
+ *     subscription stayed live across the reset, and the record landing came
+ *     back to the host as if the user had edited every field it filled — the
+ *     exact failure #2968 was filed about.
+ *
+ * Both shapes are exercised below against the same script — mount, user edit,
+ * record lands, user edit — and asserted on the same expected sequences. That
+ * symmetry IS the pin: the two runs may not disagree.
+ *
+ * ⛔ Out of scope, and deliberately not asserted here: whether these channels
+ * SHOULD report a programmatic reset. That is a contract change and #5235 left
+ * it open. These tests pin the answer the file already gave (they do not) and
+ * only remove its dependence on caller identity.
+ *
+ * Note on `onDirtyChange`: its PAYLOAD is identity-independent (always `false`
+ * for a reset that carried nothing) but its call COUNT is not — a memoized
+ * caller also hears the reset through the unconditional dirty subscription,
+ * with the same `false`. That duplicate is pre-existing, harmless and outside
+ * this card, so these tests assert the payloads rather than freezing a count
+ * that differs by caller shape.
+ */
+import { describe, it, expect } from 'vitest';
+import { render, waitFor, fireEvent, act } from '@testing-library/react';
+import React from 'react';
+import { ComponentRegistry } from '@object-ui/core';
+// Registers the renderers at module scope, NOT inside a `beforeAll` — there the
+// cold transform is billed to `hookTimeout`. See
+// object-ui/no-dynamic-import-in-test-hook (objectui#3010/#3021).
+import '../../../renderers';
+
+type Rec = Record<string, unknown>;
+type Channel = 'onChange' | 'form_change' | 'onDirtyChange';
+type Ev = { channel: Channel; payload: unknown };
+
+const FIELDS = [
+  { name: 'name', label: 'Name', type: 'input' },
+  { name: 'note', label: 'Note', type: 'input' },
+];
+
+const nameInput = (root: ParentNode) =>
+  root.querySelector('input[name="name"]') as HTMLInputElement | null;
+
+/**
+ * One host, two caller shapes. `memoized` passes callbacks with a STABLE
+ * identity (`useCallback(fn, [])` — the shape that silently turned the
+ * guarantee off); `inline` passes a fresh arrow each render (the shape the
+ * effect ordering happened to serve). Everything else is byte-identical, so a
+ * behavioural difference between the two runs can only be caller identity.
+ */
+function mountHost(shape: 'memoized' | 'inline') {
+  const events: Ev[] = [];
+  const push = (channel: Channel, payload: unknown) => { events.push({ channel, payload }); };
+  let setDefaults!: (v: Rec) => void;
+
+  const Host = () => {
+    const [defaults, setD] = React.useState<Rec>({});
+    setDefaults = setD;
+    const memoOnChange = React.useCallback((values: Rec) => { push('onChange', values); }, []);
+    const memoOnDirtyChange = React.useCallback((dirty: boolean) => { push('onDirtyChange', dirty); }, []);
+    const memoOnAction = React.useCallback((action: { type?: string; data?: Rec }) => {
+      if (action?.type === 'form_change') push('form_change', action.data);
+    }, []);
+    const Form = ComponentRegistry.get('form') as React.ComponentType<Rec>;
+    return (
+      <Form
+        schema={{
+          type: 'form',
+          fields: FIELDS,
+          defaultValues: defaults,
+          showSubmit: false,
+          onChange:
+            shape === 'memoized'
+              ? memoOnChange
+              : (values: Rec) => { push('onChange', values); },
+          onDirtyChange:
+            shape === 'memoized'
+              ? memoOnDirtyChange
+              : (dirty: boolean) => { push('onDirtyChange', dirty); },
+        }}
+        onAction={
+          shape === 'memoized'
+            ? memoOnAction
+            : (action: { type?: string; data?: Rec }) => {
+                if (action?.type === 'form_change') push('form_change', action.data);
+              }
+        }
+      />
+    );
+  };
+
+  const utils = render(<Host />);
+  const on = (channel: Channel, from = 0) =>
+    events.slice(from).filter((e) => e.channel === channel).map((e) => e.payload);
+  return { ...utils, events, on, setDefaults: (v: Rec) => setDefaults(v) };
+}
+
+describe.each(['memoized', 'inline'] as const)(
+  'form renderer — a defaultValues reset is not a user edit (%s callbacks)',
+  (shape) => {
+    it('reports the user edit, stays silent through the record landing, then reports the next edit', async () => {
+      const { container, events, on, setDefaults } = mountHost(shape);
+      await waitFor(() => {
+        if (!nameInput(container)) throw new Error('not ready');
+      });
+      await act(async () => {});
+      events.length = 0;
+
+      // ── 1. The user edits a field. All three channels report it. ──────────
+      fireEvent.change(nameInput(container)!, { target: { value: 'Typed' } });
+      await waitFor(() => expect(on('onChange').length).toBeGreaterThan(0));
+
+      // `toStrictEqual` so an extra key in the payload is a failure: the
+      // declared contract hands the host the FORM VALUES, and `note` is
+      // present-but-unset (`undefined`), not absent.
+      expect(on('onChange')).toStrictEqual([{ name: 'Typed', note: undefined }]);
+      expect(on('form_change')).toStrictEqual([{ name: 'Typed', note: undefined }]);
+      expect(on('onDirtyChange')).toStrictEqual([true]);
+
+      // ── 2. The record finishes loading and the form resets to it. ─────────
+      // The host's own data arriving, not the user typing.
+      const beforeReset = events.length;
+      await act(async () => { setDefaults({ name: 'Loaded', note: 'from server' }); });
+
+      // The reset really happened — this pins the ordering, not the absence of
+      // the reset.
+      await waitFor(() => expect(nameInput(container)!.value).toBe('Loaded'));
+
+      // ...and NOTHING about it reached the two value channels, for either
+      // caller shape. Pre-fix the memoized run collects
+      // `[{ name: 'Loaded', note: 'from server' }]` on both.
+      expect(on('onChange', beforeReset)).toEqual([]);
+      expect(on('form_change', beforeReset)).toEqual([]);
+
+      // The dirty channel is the one that IS driven explicitly, and it says the
+      // form is pristine against the record it just received. Payload-exact;
+      // see the header note on why the count is not frozen here.
+      const dirtyDuringReset = on('onDirtyChange', beforeReset);
+      expect(dirtyDuringReset.length).toBeGreaterThan(0);
+      expect(dirtyDuringReset.every((d) => d === false)).toBe(true);
+
+      // No event of any other kind slipped through in that window.
+      expect(
+        events.slice(beforeReset).every((e) => e.channel === 'onDirtyChange'),
+      ).toBe(true);
+
+      // ── 3. The channels are not left muted — the next real edit reports. ──
+      const beforeEdit = events.length;
+      fireEvent.change(nameInput(container)!, { target: { value: 'Edited' } });
+      await waitFor(() => expect(on('onChange', beforeEdit).length).toBeGreaterThan(0));
+
+      expect(on('onChange', beforeEdit)).toStrictEqual([{ name: 'Edited', note: 'from server' }]);
+      expect(on('form_change', beforeEdit)).toStrictEqual([{ name: 'Edited', note: 'from server' }]);
+      expect(on('onDirtyChange', beforeEdit)).toStrictEqual([true]);
+    });
+  },
+);

packages/components/src/renderers/form/form.tsx

@@ -1463,6 +1463,32 @@ ComponentRegistry.register('form',
     const baselineRef = React.useRef<Record<string, unknown>>(
       (defaultValues ?? {}) as Record<string, unknown>,
     );
+    // The window in which a `defaultValues` reset is running. The two
+    // value-notification channels below — `onChange` and the `form_change`
+    // `onAction` — read it, so that what they report across a reset is stated
+    // HERE, explicitly, the way `onDirtyChange` already states it (a few lines
+    // down: it computes its payload from `baselineRef` and is then called
+    // outright).
+    //
+    // Before this ref, those two rode on React's effect ordering instead:
+    // every layout DESTROY runs before any layout CREATE, so a caller passing
+    // a fresh callback each render had its subscription torn down before the
+    // reset and re-established after, and the reset went unreported. That
+    // guarantee was delivered by the CALLBACK'S IDENTITY CHANGING — so a
+    // caller wrapping the same callback in `React.useCallback`, taught
+    // everywhere as a pure performance optimization, kept one identity, the
+    // effect never re-ran, the subscription stayed live across the reset, and
+    // the record landing was delivered as if the user had edited every field
+    // it filled (#2968, measured in #5235). Nothing in the type, the docs or
+    // this renderer said "do not memoize this callback".
+    //
+    // ⛔ NOT decided here (#5235, explicitly left open): whether a value
+    // channel SHOULD report a programmatic reset. That is a contract change.
+    // This only makes the answer the file already gives identity-independent —
+    // the same for memoized and inline callers. If it is ever answered "yes",
+    // the explicit call belongs beside the `onDirtyChangeProp?.(...)` one
+    // below, not back in a subscription's teardown ordering.
+    const resetInFlightRef = React.useRef(false);
     // LAYOUT effect, deliberately — not a passive one. A passive effect runs a
     // commit LATER than the render that produced the new values, so there is a
     // window in which the new inputs are already mounted and interactive but the
@@ -1514,9 +1540,20 @@ ComponentRegistry.register('form',
       // Set the baseline before resetting: `reset`/`setValue` notify the watcher
       // below synchronously, and it reads this to compute dirtiness.
       baselineRef.current = incoming;
-      form.reset(defaultValues);
-      for (const [name, value] of carried) {
-        form.setValue(name, value, { shouldValidate: false, shouldDirty: true });
+      // Everything up to the `finally` is the reset: `reset()` itself plus the
+      // re-application of the carried values, which is part of the same
+      // operation and is no more a user edit than the reset is. RHF delivers
+      // both to `form.watch` subscribers SYNCHRONOUSLY (measured on 7.85, and
+      // relied on by the `baselineRef` line above), so the window shuts before
+      // anything else can run in it — a keystroke least of all.
+      resetInFlightRef.current = true;
+      try {
+        form.reset(defaultValues);
+        for (const [name, value] of carried) {
+          form.setValue(name, value, { shouldValidate: false, shouldDirty: true });
+        }
+      } finally {
+        resetInFlightRef.current = false;
       }
       // Fresh values, so last attempt's rejected-field markers no longer apply.
       setRejectedFieldNames([]);
@@ -1530,17 +1567,21 @@ ComponentRegistry.register('form',
 
     // Watch for form changes - only track changes when onAction is available.
     // LAYOUT effect to stay in the same phase as the `defaultValues` reset
-    // above. React runs every layout DESTROY (mutation phase) before any layout
-    // CREATE, so a caller passing a fresh `onAction` each render — the common
-    // case, it is usually an inline arrow — has this subscription torn down
-    // before the reset runs and re-established after. That is what keeps a
-    // reset from being reported as a user edit. Leaving this passive while the
-    // reset is layout-phase inverts the order: the reset fires into the still
-    // live previous subscription and a record landing looks like the user
-    // editing every field it filled (#2968).
+    // above: a passive subscription is established one commit LATER than the
+    // layout-phase reset, which is a schedule of its own for no reason.
+    //
+    // What keeps a reset from being reported as a user edit is the explicit
+    // window (`resetInFlightRef`), NOT this effect's teardown ordering. That
+    // ordering unsubscribes only when the callback's identity changes, so it
+    // held for inline arrows and quietly did nothing for a caller who
+    // memoized — see the ref's declaration above (#2968, #5235).
     React.useLayoutEffect(() => {
       if (onAction) {
         const subscription = form.watch((data) => {
+          // A `defaultValues` reset is the host's own data landing, not the
+          // user editing every field it fills. Driven from the reset itself,
+          // so it holds for every caller.
+          if (resetInFlightRef.current) return;
           onAction({
             type: 'form_change',
             data,
@@ -1580,15 +1621,17 @@ ComponentRegistry.register('form',
     // nothing — a behaviour change for them, where honouring the declaration
     // is meant to be purely additive.
     //
-    // Layout-phase for the ordering reason spelled out above the `onAction`
-    // effect: React runs every layout DESTROY before any layout CREATE, so a
-    // caller passing a fresh inline arrow each render — the common case — has
-    // this torn down before the `defaultValues` reset runs and re-established
-    // after. That is what keeps a record landing from being reported to the
-    // host as a user edit. A passive effect inverts the order (#2968).
+    // Layout-phase for the same reason as the `onAction` effect above, and —
+    // like it — a `defaultValues` reset is kept off this channel by the
+    // explicit window (`resetInFlightRef`), not by whether React happened to
+    // tear this subscription down. The teardown only happens when the
+    // callback's identity changes, i.e. for callers who do not memoize; the
+    // guarantee is not theirs alone (#2968, #5235).
     React.useLayoutEffect(() => {
       if (onChangeProp) {
         const subscription = form.watch((values) => {
+          // The host's own record landing is not the user changing values.
+          if (resetInFlightRef.current) return;
           onChangeProp(values as Record<string, any>);
         });
         return () => subscription.unsubscribe();