From 12f52940820a990e29a90b3fed047e3bf8a491f2 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:45:47 +0800 Subject: [PATCH] fix(form): a defaultValues change no longer discards the field being filled (#2982) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The form renderer adopts a changed `defaultValues` with `form.reset()`, which replaces the WHOLE react-hook-form record — so it also blanks every field the incoming defaults say nothing about. And it runs in a PASSIVE effect, one commit after those fields were committed and painted, so input landing in that window was silently discarded. PR #2983 fixed only the test that caught it. This is the product-side hole it left open. The wizard 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 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 all three load-bearing paths keep today's behavior exactly: - an edit-mode record landing after first paint still fills every field it names — an untouched field is empty-ish against the baseline under the same comparison the dirty check uses, so a widget normalizing its own empty value on mount ('' -> null) is not mistaken for input. This is why RHF's `dirtyFields`/`keepDirtyValues` could not be used: it flags exactly those self-normalizing fields, and would have rejected the loaded record for them; - 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 carried input also now reports dirty (it is, against the caller's defaults) rather than unconditionally announcing pristine, so a host's discard guard keeps hearing the truth. Measured, pre-#2983 harness under CPU load, 400 iterations per arm: before 5 failures (each `{"name":"Alice"}`, note lost), after 0. New renderer-level test pins the fix and the three paths above deterministically, without timing games. Verified: `npx vitest run packages/plugin-form/ packages/components/` — 68 files / 577 tests pass; `@object-ui/components` type-check and lint clean. Co-Authored-By: Claude Opus 5 --- .../form-defaults-reset-keeps-user-input.md | 47 +++++++ ...-defaults-change-keeps-user-input.test.tsx | 129 ++++++++++++++++++ .../components/src/renderers/form/form.tsx | 51 ++++++- 3 files changed, 223 insertions(+), 4 deletions(-) create mode 100644 .changeset/form-defaults-reset-keeps-user-input.md create mode 100644 packages/components/src/renderers/form/__tests__/form-defaults-change-keeps-user-input.test.tsx 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 17f1e6fd0..a73dd7faf 100644 --- a/packages/components/src/renderers/form/form.tsx +++ b/packages/components/src/renderers/form/form.tsx @@ -109,6 +109,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, @@ -466,11 +470,50 @@ 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; - // A fresh reset is by definition pristine — clear any stale dirty signal - // (e.g. an edit-mode record that just finished loading). - onDirtyChangeProp?.(false); + for (const [name, value] of carried) { + form.setValue(name, value, { shouldValidate: false, shouldDirty: true }); + } + // 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]);