Skip to content

Commit c769d3d

Browse files
os-zhuangclaude
andauthored
fix(form): a defaultValues change no longer discards the field being filled (#2982) (#2991)
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: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 6f29aa5 commit c769d3d

3 files changed

Lines changed: 223 additions & 4 deletions

File tree

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
---
2+
"@object-ui/components": patch
3+
---
4+
5+
fix(form): a `defaultValues` change no longer discards the field the user is filling
6+
7+
The form renderer adopts a changed `defaultValues` with `form.reset()`, which
8+
replaces the **whole** react-hook-form record — so it also blanks the fields the
9+
incoming defaults say nothing about. And it runs in a **passive effect**, one
10+
commit after those fields have been committed and painted, so input landing in
11+
that window was silently dropped.
12+
13+
The caught case is the wizard (objectui#2982). It reuses ONE inner form across
14+
steps and feeds it `defaultValues={formData}` — the merge of the steps submitted
15+
**so far** — so at every step boundary the incoming defaults are missing exactly
16+
the fields now on screen:
17+
18+
```
19+
RESET to {"name":"Alice"} (values before: {"name":"Alice","note":"hello"})
20+
-> create POST {"name":"Alice"} — the last step is gone
21+
```
22+
23+
In a browser this needs a busy main thread plus typing on the first frame after
24+
the new defaults arrive — unlikely by hand, but paste and autofill land in a
25+
single tick. The same shape had already bitten once before, as a `reset()` on
26+
`defaultValues` **identity** churn wiping input mid-interaction; comparing by
27+
value fixed that, and this is the residual hole where the value genuinely did
28+
change.
29+
30+
The reset now carries such a value across instead of dropping it. Deliberately
31+
narrow: only a field the **caller has never carried** — absent from both the
32+
outgoing and the incoming defaults — and whose value the user actually changed
33+
is eligible. Wherever the caller has an opinion it stays authoritative, so the
34+
load-bearing paths are unchanged:
35+
36+
- an edit-mode record landing after first paint still fills every field it names
37+
(a field the user has NOT touched is empty-ish against the baseline, using the
38+
same comparison the dirty check uses, so a widget normalizing its own empty
39+
value on mount is not mistaken for input);
40+
- a `recordId` swap still replaces the record outright — drawer/modal/split
41+
forms re-fetch without re-entering their loading branch, so record B lands in
42+
the still-mounted form and must not inherit an abandoned edit to record A;
43+
- a field the caller withdraws from its defaults stops being the user's.
44+
45+
A reset that carries input now also reports the form as dirty (it is, against
46+
the caller's defaults) instead of unconditionally announcing pristine, so a
47+
host's discard guard keeps hearing the truth.
Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* A `defaultValues` change must not throw away what the user typed (#2982).
11+
*
12+
* The renderer adopts a changed `defaultValues` with `form.reset()`, which
13+
* replaces the WHOLE react-hook-form record. Any field the incoming defaults do
14+
* not mention is therefore blanked — including one the user just filled in. The
15+
* caught case was the wizard: it reuses ONE inner form across steps and feeds it
16+
* `defaultValues={formData}`, the merged data of the steps SUBMITTED SO FAR. So
17+
* on every step boundary the incoming defaults are missing exactly the fields of
18+
* the step now on screen, and the create POST went out without them:
19+
*
20+
* RESET to {"name":"Alice"} (values before: {"name":"Alice","note":"hello"})
21+
* -> create {"name":"Alice"} — the last step is gone
22+
*
23+
* (The reset runs in a passive effect, one commit after the new step's inputs
24+
* are painted, so a real user can type inside that window too — and paste or
25+
* autofill lands there in a single tick.)
26+
*
27+
* The rule pinned here: the caller's defaults stay authoritative for every field
28+
* the caller has an opinion about — a landing record, a swapped record, a
29+
* dropped field all overwrite freely, exactly as before. Only a value the caller
30+
* has NEVER carried (absent from both the outgoing and the incoming defaults)
31+
* and that the user actually changed survives the reset.
32+
*/
33+
import { describe, it, expect, beforeAll, vi } from 'vitest';
34+
import { render, fireEvent, waitFor } from '@testing-library/react';
35+
import { ComponentRegistry } from '@object-ui/core';
36+
37+
beforeAll(async () => {
38+
await import('../../../renderers');
39+
}, 30000);
40+
41+
const fields = [
42+
{ name: 'name', label: 'Name', type: 'input' },
43+
{ name: 'note', label: 'Note', type: 'input' },
44+
];
45+
46+
function formElement(
47+
defaults: Record<string, unknown>,
48+
onSubmit?: (data: Record<string, unknown>) => void,
49+
) {
50+
const Form = ComponentRegistry.get('form')!;
51+
return (
52+
<Form
53+
schema={{
54+
type: 'form',
55+
fields,
56+
defaultValues: defaults,
57+
showSubmit: false,
58+
showCancel: false,
59+
onSubmit,
60+
}}
61+
/>
62+
);
63+
}
64+
65+
const input = (c: HTMLElement, name: string) =>
66+
c.querySelector(`input[name="${name}"]`) as HTMLInputElement;
67+
68+
describe('form renderer — a defaultValues change keeps user input (#2982)', () => {
69+
it('keeps a field the incoming defaults never mention, and submits it', async () => {
70+
const onSubmit = vi.fn();
71+
const { rerender, container } = render(formElement({}, onSubmit));
72+
73+
// The user fills the field that is on screen now. The caller does not model
74+
// it yet — for the wizard, `note` belongs to the step being filled.
75+
fireEvent.change(input(container, 'note'), { target: { value: 'hello' } });
76+
77+
// The caller's defaults change to what it HAS collected (step 1's `name`).
78+
rerender(formElement({ name: 'Alice' }, onSubmit));
79+
80+
expect(input(container, 'note').value).toBe('hello');
81+
expect(input(container, 'name').value).toBe('Alice');
82+
83+
// The harm was on the wire, not just on screen: the create body lost a step.
84+
fireEvent.submit(container.querySelector('form') as HTMLFormElement);
85+
await waitFor(() => expect(onSubmit).toHaveBeenCalled());
86+
expect(onSubmit.mock.calls[0][0]).toEqual(
87+
expect.objectContaining({ name: 'Alice', note: 'hello' }),
88+
);
89+
});
90+
91+
it('still adopts a record that lands after first paint (edit mode)', () => {
92+
// The load-bearing case the reset exists for: an edit form paints before
93+
// `findOne` resolves, and every field must take the record's values.
94+
const { rerender, container } = render(formElement({}));
95+
96+
rerender(formElement({ name: 'Alice', note: 'from the record' }));
97+
98+
expect(input(container, 'name').value).toBe('Alice');
99+
expect(input(container, 'note').value).toBe('from the record');
100+
});
101+
102+
it('does not leak an unsaved edit into a different record swapped in place', () => {
103+
// Split/drawer/modal forms re-fetch on a `recordId` change WITHOUT going
104+
// back through their loading branch, so record B lands in the still-mounted
105+
// form. B's values must win outright — carrying A's abandoned edit across
106+
// would silently write it to the wrong record.
107+
const { rerender, container } = render(formElement({ name: 'A', note: 'note A' }));
108+
109+
fireEvent.change(input(container, 'note'), { target: { value: 'unsaved edit' } });
110+
rerender(formElement({ name: 'B', note: 'note B' }));
111+
112+
expect(input(container, 'name').value).toBe('B');
113+
expect(input(container, 'note').value).toBe('note B');
114+
});
115+
116+
it('still lets the caller take back a field it drops from its defaults', () => {
117+
// The caller previously carried `note` and no longer does: that is the
118+
// caller withdrawing the value, so the user's edit does not survive it.
119+
// (react-hook-form reverts a dropped key to the default it last had, rather
120+
// than blanking it — the point asserted here is only that the caller, not
121+
// the abandoned edit, decides.)
122+
const { rerender, container } = render(formElement({ name: 'Alice', note: 'note A' }));
123+
124+
fireEvent.change(input(container, 'note'), { target: { value: 'unsaved edit' } });
125+
rerender(formElement({ name: 'Alice' }));
126+
127+
expect(input(container, 'note').value).toBe('note A');
128+
});
129+
});

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

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,10 @@ const valuesEqualForDirty = (a: unknown, b: unknown): boolean => {
119119
}
120120
};
121121

122+
/** Own-property test — a field can legitimately be named `constructor`. */
123+
const hasOwn = (o: Record<string, unknown>, k: string): boolean =>
124+
Object.prototype.hasOwnProperty.call(o ?? {}, k);
125+
122126
const computeDirty = (
123127
baseline: Record<string, unknown>,
124128
values: Record<string, unknown>,
@@ -561,13 +565,52 @@ ComponentRegistry.register('form',
561565
try { key = JSON.stringify(defaultValues ?? {}); } catch { key = String(Date.now()); }
562566
if (lastDefaultsKey.current === key) return;
563567
lastDefaultsKey.current = key;
568+
const incoming = (defaultValues ?? {}) as Record<string, unknown>;
569+
const outgoing = baselineRef.current;
570+
// `reset()` replaces the WHOLE record, so it also blanks fields the
571+
// incoming defaults say nothing about — and it lands here in a PASSIVE
572+
// effect, one commit after those fields were painted. Input arriving in
573+
// that window was silently discarded. The caught case is the wizard: it
574+
// reuses one inner form across steps and feeds back `formData`, the merge
575+
// of the steps submitted SO FAR — so at every step boundary the incoming
576+
// defaults are missing exactly the fields now on screen, and the create
577+
// POST went out without the step just filled (#2982):
578+
//
579+
// RESET to {"name":"Alice"} (values before: {"name":"Alice","note":"hello"})
580+
//
581+
// Carry such a value across the reset instead of dropping it.
582+
const carried = Object.entries(form.getValues() as Record<string, unknown>).filter(
583+
([name, value]) =>
584+
// Only a field the CALLER HAS NEVER CARRIED is eligible — absent from
585+
// both the outgoing and the incoming defaults. Everywhere the caller
586+
// has an opinion it stays authoritative, so the three load-bearing
587+
// paths keep today's behavior exactly: a record landing after first
588+
// paint still fills every field it names; a `recordId` swap still
589+
// replaces the record outright (drawer/modal/split forms re-fetch
590+
// without re-entering their loading branch, so record B lands in the
591+
// still-mounted form and must not inherit an abandoned edit to A);
592+
// and a field the caller withdraws stops being the user's.
593+
!hasOwn(incoming, name) &&
594+
!hasOwn(outgoing, name) &&
595+
// The same empty-ish comparison the dirty check uses, so a widget
596+
// normalizing its own empty value on mount ('' -> null, undefined ->
597+
// '') is not mistaken for input and does not keep a field alive.
598+
!valuesEqualForDirty(outgoing[name], value),
599+
);
600+
// Set the baseline before resetting: `reset`/`setValue` notify the watcher
601+
// below synchronously, and it reads this to compute dirtiness.
602+
baselineRef.current = incoming;
564603
form.reset(defaultValues);
565-
baselineRef.current = (defaultValues ?? {}) as Record<string, unknown>;
604+
for (const [name, value] of carried) {
605+
form.setValue(name, value, { shouldValidate: false, shouldDirty: true });
606+
}
566607
// Fresh values, so last attempt's rejected-field markers no longer apply.
567608
setRejectedFieldNames([]);
568-
// A fresh reset is by definition pristine — clear any stale dirty signal
569-
// (e.g. an edit-mode record that just finished loading).
570-
onDirtyChangeProp?.(false);
609+
// A reset that carried nothing is by definition pristine — clear any stale
610+
// dirty signal (e.g. an edit-mode record that just finished loading). One
611+
// that carried input is genuinely dirty against the caller's defaults, and
612+
// the host's discard guard has to keep hearing so.
613+
onDirtyChangeProp?.(carried.length > 0);
571614
// eslint-disable-next-line react-hooks/exhaustive-deps
572615
}, [defaultValues]);
573616

0 commit comments

Comments
 (0)