diff --git a/.changeset/form-defaults-reset-keeps-user-input.md b/.changeset/form-defaults-reset-keeps-user-input.md new file mode 100644 index 000000000..dd5b80a9d --- /dev/null +++ b/.changeset/form-defaults-reset-keeps-user-input.md @@ -0,0 +1,47 @@ +--- +"@object-ui/components": patch +--- + +fix(form): a `defaultValues` change no longer discards the field the user is filling + +The form renderer adopts a changed `defaultValues` with `form.reset()`, which +replaces the **whole** react-hook-form record — so it also blanks the fields the +incoming defaults say nothing about. And it runs in a **passive effect**, one +commit after those fields have been committed and painted, so input landing in +that window was silently dropped. + +The caught case is the wizard (objectui#2982). It reuses ONE inner form across +steps and feeds it `defaultValues={formData}` — the merge of the steps submitted +**so far** — so at every step boundary the incoming defaults are missing exactly +the fields now on screen: + +``` +RESET to {"name":"Alice"} (values before: {"name":"Alice","note":"hello"}) +-> create POST {"name":"Alice"} — the last step is gone +``` + +In a browser this needs a busy main thread plus typing on the first frame after +the new defaults arrive — unlikely by hand, but paste and autofill land in a +single tick. The same shape had already bitten once before, as a `reset()` on +`defaultValues` **identity** churn wiping input mid-interaction; comparing by +value fixed that, and this is the residual hole where the value genuinely did +change. + +The reset now carries such a value across instead of dropping it. Deliberately +narrow: only a field the **caller has never carried** — absent from both the +outgoing and the incoming defaults — and whose value the user actually changed +is eligible. Wherever the caller has an opinion it stays authoritative, so the +load-bearing paths are unchanged: + +- an edit-mode record landing after first paint still fills every field it names + (a field the user has NOT touched is empty-ish against the baseline, using the + same comparison the dirty check uses, so a widget normalizing its own empty + value on mount is not mistaken for input); +- a `recordId` swap still replaces the record outright — drawer/modal/split + forms re-fetch without re-entering their loading branch, so record B lands in + the still-mounted form and must not inherit an abandoned edit to record A; +- a field the caller withdraws from its defaults stops being the user's. + +A reset that carries input now also reports the form as dirty (it is, against +the caller's defaults) instead of unconditionally announcing pristine, so a +host's discard guard keeps hearing the truth. diff --git a/packages/components/src/renderers/form/__tests__/form-defaults-change-keeps-user-input.test.tsx b/packages/components/src/renderers/form/__tests__/form-defaults-change-keeps-user-input.test.tsx new file mode 100644 index 000000000..4a52b08d1 --- /dev/null +++ b/packages/components/src/renderers/form/__tests__/form-defaults-change-keeps-user-input.test.tsx @@ -0,0 +1,129 @@ +/** + * 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` change must not throw away what the user typed (#2982). + * + * The renderer adopts a changed `defaultValues` with `form.reset()`, which + * replaces the WHOLE react-hook-form record. Any field the incoming defaults do + * not mention is therefore blanked — including one the user just filled in. The + * caught case was the wizard: it reuses ONE inner form across steps and feeds it + * `defaultValues={formData}`, the merged data of the steps SUBMITTED SO FAR. So + * on every step boundary the incoming defaults are missing exactly the fields of + * the step now on screen, and the create POST went out without them: + * + * RESET to {"name":"Alice"} (values before: {"name":"Alice","note":"hello"}) + * -> create {"name":"Alice"} — the last step is gone + * + * (The reset runs in a passive effect, one commit after the new step's inputs + * are painted, so a real user can type inside that window too — and paste or + * autofill lands there in a single tick.) + * + * The rule pinned here: the caller's defaults stay authoritative for every field + * the caller has an opinion about — a landing record, a swapped record, a + * dropped field all overwrite freely, exactly as before. Only a value the caller + * has NEVER carried (absent from both the outgoing and the incoming defaults) + * and that the user actually changed survives the reset. + */ +import { describe, it, expect, beforeAll, vi } from 'vitest'; +import { render, fireEvent, waitFor } from '@testing-library/react'; +import { ComponentRegistry } from '@object-ui/core'; + +beforeAll(async () => { + await import('../../../renderers'); +}, 30000); + +const fields = [ + { name: 'name', label: 'Name', type: 'input' }, + { name: 'note', label: 'Note', type: 'input' }, +]; + +function formElement( + defaults: Record, + onSubmit?: (data: Record) => void, +) { + const Form = ComponentRegistry.get('form')!; + return ( +
+ ); +} + +const input = (c: HTMLElement, name: string) => + c.querySelector(`input[name="${name}"]`) as HTMLInputElement; + +describe('form renderer — a defaultValues change keeps user input (#2982)', () => { + it('keeps a field the incoming defaults never mention, and submits it', async () => { + const onSubmit = vi.fn(); + const { rerender, container } = render(formElement({}, onSubmit)); + + // The user fills the field that is on screen now. The caller does not model + // it yet — for the wizard, `note` belongs to the step being filled. + fireEvent.change(input(container, 'note'), { target: { value: 'hello' } }); + + // The caller's defaults change to what it HAS collected (step 1's `name`). + rerender(formElement({ name: 'Alice' }, onSubmit)); + + expect(input(container, 'note').value).toBe('hello'); + expect(input(container, 'name').value).toBe('Alice'); + + // The harm was on the wire, not just on screen: the create body lost a step. + fireEvent.submit(container.querySelector('form') as HTMLFormElement); + await waitFor(() => expect(onSubmit).toHaveBeenCalled()); + expect(onSubmit.mock.calls[0][0]).toEqual( + expect.objectContaining({ name: 'Alice', note: 'hello' }), + ); + }); + + it('still adopts a record that lands after first paint (edit mode)', () => { + // The load-bearing case the reset exists for: an edit form paints before + // `findOne` resolves, and every field must take the record's values. + const { rerender, container } = render(formElement({})); + + rerender(formElement({ name: 'Alice', note: 'from the record' })); + + expect(input(container, 'name').value).toBe('Alice'); + expect(input(container, 'note').value).toBe('from the record'); + }); + + it('does not leak an unsaved edit into a different record swapped in place', () => { + // Split/drawer/modal forms re-fetch on a `recordId` change WITHOUT going + // back through their loading branch, so record B lands in the still-mounted + // form. B's values must win outright — carrying A's abandoned edit across + // would silently write it to the wrong record. + const { rerender, container } = render(formElement({ name: 'A', note: 'note A' })); + + fireEvent.change(input(container, 'note'), { target: { value: 'unsaved edit' } }); + rerender(formElement({ name: 'B', note: 'note B' })); + + expect(input(container, 'name').value).toBe('B'); + expect(input(container, 'note').value).toBe('note B'); + }); + + it('still lets the caller take back a field it drops from its defaults', () => { + // The caller previously carried `note` and no longer does: that is the + // caller withdrawing the value, so the user's edit does not survive it. + // (react-hook-form reverts a dropped key to the default it last had, rather + // than blanking it — the point asserted here is only that the caller, not + // the abandoned edit, decides.) + const { rerender, container } = render(formElement({ name: 'Alice', note: 'note A' })); + + fireEvent.change(input(container, 'note'), { target: { value: 'unsaved edit' } }); + rerender(formElement({ name: 'Alice' })); + + expect(input(container, 'note').value).toBe('note A'); + }); +}); diff --git a/packages/components/src/renderers/form/form.tsx b/packages/components/src/renderers/form/form.tsx index a78c03df2..e03c1e65e 100644 --- a/packages/components/src/renderers/form/form.tsx +++ b/packages/components/src/renderers/form/form.tsx @@ -119,6 +119,10 @@ const valuesEqualForDirty = (a: unknown, b: unknown): boolean => { } }; +/** Own-property test — a field can legitimately be named `constructor`. */ +const hasOwn = (o: Record, k: string): boolean => + Object.prototype.hasOwnProperty.call(o ?? {}, k); + const computeDirty = ( baseline: Record, values: Record, @@ -561,13 +565,52 @@ ComponentRegistry.register('form', try { key = JSON.stringify(defaultValues ?? {}); } catch { key = String(Date.now()); } if (lastDefaultsKey.current === key) return; lastDefaultsKey.current = key; + const incoming = (defaultValues ?? {}) as Record; + const outgoing = baselineRef.current; + // `reset()` replaces the WHOLE record, so it also blanks fields the + // incoming defaults say nothing about — and it lands here in a PASSIVE + // effect, one commit after those fields were painted. Input arriving in + // that window was silently discarded. The caught case is the wizard: it + // reuses one inner form across steps and feeds back `formData`, the merge + // of the steps submitted SO FAR — so at every step boundary the incoming + // defaults are missing exactly the fields now on screen, and the create + // POST went out without the step just filled (#2982): + // + // RESET to {"name":"Alice"} (values before: {"name":"Alice","note":"hello"}) + // + // Carry such a value across the reset instead of dropping it. + const carried = Object.entries(form.getValues() as Record).filter( + ([name, value]) => + // Only a field the CALLER HAS NEVER CARRIED is eligible — absent from + // both the outgoing and the incoming defaults. Everywhere the caller + // has an opinion it stays authoritative, so the three load-bearing + // paths keep today's behavior exactly: a record landing after first + // paint still fills every field it names; a `recordId` swap still + // replaces the record outright (drawer/modal/split forms re-fetch + // without re-entering their loading branch, so record B lands in the + // still-mounted form and must not inherit an abandoned edit to A); + // and a field the caller withdraws stops being the user's. + !hasOwn(incoming, name) && + !hasOwn(outgoing, name) && + // The same empty-ish comparison the dirty check uses, so a widget + // normalizing its own empty value on mount ('' -> null, undefined -> + // '') is not mistaken for input and does not keep a field alive. + !valuesEqualForDirty(outgoing[name], value), + ); + // 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); - baselineRef.current = (defaultValues ?? {}) as Record; + for (const [name, value] of carried) { + form.setValue(name, value, { shouldValidate: false, shouldDirty: true }); + } // Fresh values, so last attempt's rejected-field markers no longer apply. setRejectedFieldNames([]); - // A fresh reset is by definition pristine — clear any stale dirty signal - // (e.g. an edit-mode record that just finished loading). - onDirtyChangeProp?.(false); + // A reset that carried nothing is by definition pristine — clear any stale + // dirty signal (e.g. an edit-mode record that just finished loading). One + // that carried input is genuinely dirty against the caller's defaults, and + // the host's discard guard has to keep hearing so. + onDirtyChangeProp?.(carried.length > 0); // eslint-disable-next-line react-hooks/exhaustive-deps }, [defaultValues]);