diff --git a/.changeset/form-reset-notification-channels-5235.md b/.changeset/form-reset-notification-channels-5235.md new file mode 100644 index 000000000..6a2d2c3f1 --- /dev/null +++ b/.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. diff --git a/packages/components/src/renderers/form/__tests__/form-reset-notification-channels.test.tsx b/packages/components/src/renderers/form/__tests__/form-reset-notification-channels.test.tsx new file mode 100644 index 000000000..354a4846e --- /dev/null +++ b/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; +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({}); + 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; + return ( +
{ 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(); + 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]); + }); + }, +); diff --git a/packages/components/src/renderers/form/form.tsx b/packages/components/src/renderers/form/form.tsx index 419b941fa..5c4975aab 100644 --- a/packages/components/src/renderers/form/form.tsx +++ b/packages/components/src/renderers/form/form.tsx @@ -1463,6 +1463,32 @@ ComponentRegistry.register('form', const baselineRef = React.useRef>( (defaultValues ?? {}) as Record, ); + // 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); }); return () => subscription.unsubscribe();