Uh oh!
There was an error while loading. Please reload this page.
test(plugin-form): settle each wizard step before typing (#2982) - #2983
Merged
Conversation
`WizardForm.regression` "submits the merged payload" flaked on CI: the create
POST occasionally carried `{ name }` only, dropping the last step's field.
The step form is REUSED across steps (one react-hook-form instance) and the
wizard feeds it `defaultValues={formData}`. A change to that value is applied
with `reset()` in a passive effect — one commit AFTER the new step's inputs are
already in the DOM. `waitFor(input)` can resolve inside that window, and a value
typed there is lost: the pending reset replaces the whole RHF record with the
earlier steps' data. Captured interleaving:
test: waitFor(note) RESOLVED
form: RESET to {"name":"Alice"} (values before: {"name":"Alice","note":"hello"})
test: change note done
→ payload {"name":"Alice"}
Note the loss lands BEFORE the submit, so waiting *after* the change (as the
issue title suggests) would not have helped — the step has to be settled before
typing. `settledStepInput()` drains the pending effects after the input mounts.
Harness-only: nothing asserted changes, and the guarded regression still bites —
breaking both value carriers (RHF retention + the wizard's cross-step merge)
fails old and new test identically with `{ note: 'hello' }`.
Measured under CPU load, 1500 iterations each: before 7 failures, after 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jul 30, 2026
os-zhuang added a commit
that referenced
this pull request
Jul 30, 2026
…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>
os-zhuang added a commit
that referenced
this pull request
Jul 30, 2026
…ers them (#3001) The form applies a `defaultValues` change by resetting react-hook-form to it. While that ran in a PASSIVE effect there was a window: the render had already committed, so the new inputs were mounted and interactive, but the form still held the old record. Anything typed in that window was destroyed — the pending `reset()` overwrote the whole record with `defaultValues`, dropping the field the user had just filled, with no error and nothing in the payload. It surfaced as the flaky wizard test fixed in #2983: a step transition changes `defaultValues`, and `note` typed on the new step vanished from the create body. That fix drained pending effects in the test, which AVOIDS the window; this one CLOSES it. Measured with the pre-fix test pattern, 1500 replays under CPU load: 6 failures before, 0 after. Running the reset as a layout effect is only half of it. The `form_change` subscription was silent across a reset by accident of ordering, not by design: `onAction` is usually an inline arrow, so its identity changes every render and the subscription effect re-runs each commit — and React runs every passive DESTROY before any passive CREATE, so the watcher was unsubscribed before the reset fired. Hoisting only the reset to the layout phase puts it AHEAD of that cleanup, so the still-live previous subscription sees it and a record landing looks like the user having edited every field it filled. That regressed #2968 deterministically (`changes=[{"category":"not-offered","status":"pending"}]`). Both `form.watch` subscriptions therefore move to the same phase, restoring the destroy-then-create order exactly. The new test pins the window shut without depending on timing: React flushes passive effects in tree order, so a probe sibling rendered before the form is guaranteed to run inside the window and types from there. It fails on every run against the previous code. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#2982.
The flake
WizardForm.regression→ "submits the merged payload" occasionally saw the create POST carry{ name: 'Alice' }only, dropping the last step'snote.The inner step form is reused across steps (one
react-hook-forminstance), andWizardFormfeeds itdefaultValues={formData}. When that value changes, the form applies it withreset()in a passive effect — i.e. one commit after the new step's inputs are already in the DOM.waitFor(input)can resolve inside that window, and a value typed there is lost: the pending reset replaces the whole RHF record with the earlier steps' data.Captured interleaving (instrumented run):
The issue title's diagnosis is slightly off, and it matters: the value is lost before the submit, during the
change. Waiting afterfireEvent.change— the fix the title suggests — would not have helped. The step has to be settled before typing.The fix
A
settledStepInput()helper that waits for the step's input and drains the pending effects, so thedefaultValuesreset cannot land after the change. Applied to both steps (step 1 has the same window against the initial mount reset).Why this doesn't weaken the guard
Harness-only — nothing asserted changed. Verified by mutation: breaking both value carriers (RHF cross-step retention + the wizard's
{...formData, ...stepData}merge) fails the old and the new test identically, with the exact{ note: 'hello' }"last-only body" the file's docblock names.Worth noting the two carriers are redundant on their own — breaking either one alone still passes.
Verification
Scenario replayed 1500× per arm in-process, under 10 competing CPU hogs:
Full
plugin-formsuite: 24 files / 217 tests pass. No newtscoreslintfindings in the changed file.Follow-up (not in this PR)
The same window exists in the product, narrowly:
form.tsxcommits newdefaultValuesone frame before itsreset()runs, so input typed in that gap is silently discarded. In a browser it takes a busy main thread plus typing on the very first frame of a step, so it is far less likely than in tests — but it is the same defect, and it lives in a shared renderer every form uses. Deliberately left alone here to keep this change harness-only.🤖 Generated with Claude Code