Form renderer: a defaultValues reset is not a user edit for memoized callers either - #5707
Merged
os-sales merged 2 commits intoAug 22, 2026
Merged
Conversation
… by callback identity A `defaultValues` reset kept off `onChange` and off the `form_change` `onAction` only for callers who do NOT memoize: the guarantee rode on React running every layout DESTROY before any layout CREATE, so it was delivered by the callback's identity changing. `React.useCallback` kept one identity, the effect never re-ran, the subscription survived the reset, and a landing record came back to the host as a user edit (objectui#2968, measured in objectui#5235). The reset now holds an explicit window that those two channels read, the shape `onDirtyChange` already had beside them. Inline-arrow callers are unaffected; memoized callers stop receiving a phantom edit. Whether a value channel SHOULD report a programmatic reset is a contract question and stays open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012u2pRjcqAYtoEjgr3wwhnK
Collaborator
Author
|
Seat review — accepted on its merits. HELD only on #5705, which is a red base, not this diff. This found a sharper defect than the card described, and the distinction matters. The card said the two value channels "ride on incidental state changes". The measured mechanism is more specific and worse: the guarantee was delivered by the callback's identity changing. React runs every layout DESTROY before any layout CREATE, so a caller passing a fresh inline arrow each render had its subscription torn down before the reset and re-established after — and the reset went unreported. A caller who wraps the same callback in So The out-of-scope line I drew at dispatch was honoured, and — better — it is now written into the source rather than living only in a card:
That last clause is the valuable half: it tells whoever answers the open question where the answer goes, so the next person cannot re-implement this as effect-ordering by accident. The obvious objection to an in-flight boolean — that it could swallow a real edit — is addressed by measurement rather than by assertion: RHF delivers both Renders and asserts the channel payloads rather than the types, per the dispatch: "reports the user edit, stays silent through the record landing, then reports the next edit" — the full sequence, not just the silence. Held pending #5705 ( Generated by Claude Code |
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-sales
marked this pull request as ready for review
August 22, 2026 13:12
os-sales
deleted the
claude/issue-5235-form-reset-notification-channels
branch
August 22, 2026 13:13
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 #5235
The asymmetry this fixes
packages/components/src/renderers/form/form.tsxpublishes three notification channels around adefaultValuesreset, and they were driven two different ways:onDirtyChangeonChangeonAction({ type: 'form_change' })The load-bearing words in the old comments were "a fresh callback each render". The guarantee was delivered by the callback's identity changing. A caller wrapping the same callback in
React.useCallback— taught everywhere as a semantically neutral performance optimization — keeps one identity, 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 every field of it. That is the false "the user edited this" signal #2968 was filed about, arriving through a door no type, doc or call site mentions.The change
One file, one ref. The reset now holds an explicit window (
resetInFlightRef) acrossform.reset(defaultValues)and the re-application of carried input (part of the same operation), and the two subscriptions read it:react-hook-form delivers both to
form.watchsubscribers synchronously (measured on the pinned 7.85 — thebaselineRefline directly above already relies on it), so the window shuts before anything else can run inside it. No subscription is added or removed: theform-onchange-wiringratchet that a callback-free form establishes exactly one value subscription is untouched and still green.onDirtyChangeis deliberately left exactly as it was — it is the channel that was already correct, and this card fixes how the other two are driven, not what any of them mean.What this does NOT decide
Whether a value channel should report a programmatic reset. #5235 left that fork open explicitly and it is a contract change; this only removes the existing answer's dependence on caller identity, so memoized and inline callers get the same one. The renderer comment says where the explicit call would go if that fork is ever answered "yes".
Also unchanged: the two
form.reset()calls on submit/cancel (different operations, reported today, reported after this), andFormSchema.onChange's JSDoc in@object-ui/types, which is silent about programmatic resets — documenting it would pre-empt the open fork, and it is a different package.Evidence
New test:
packages/components/src/renderers/form/__tests__/form-reset-notification-channels.test.tsx. It mounts the form, drives the full script in a real render — user edit → record lands → user edit — and asserts the exact per-channel sequences and payloads,describe.eachover both caller shapes against the same expectations. The symmetry is the pin: the two runs may not disagree.The gauge can fail, proven, not assumed. Ablation: with the fix committed, the renderer was checked out from
origin/main(confirmed on disk: theresetInFlightRefmarker count went 7 → 0 and the old comment text came back), then the same test re-run:The memoized leg fails with exactly the payload #5235 measured; the inline leg stays green — the predicted direction, and the reason the old pins could not see this. The file was then restored by checking that same path back out of this branch (marker back to 7,
git diff HEADempty) before any further measurement, and every reading below was taken on the restored tree.Gates run locally — all at
e856c74a7(tree clean against that commit)vitest run packages/components/ packages/fields/ packages/plugin-form/(--maxWorkers=2)pnpm --filter '@object-ui/components^...' buildthenpnpm --filter @object-ui/components type-checktsc --noEmit && tsc -p tsconfig.test.json, script name echoed)eslinton both changed files (--format json)form.tsx60 warnings — identical toorigin/main's copy of the same file measured the same way, delta 0node scripts/check-changeset-presence.mjsWhy the test scope is a provable superset, not a guess. The gate can only alter behaviour while
resetInFlightRefis true, i.e. inside thedefaultValuesreset window, and only for the two gated subscriptions — which require, respectively, anonActionprop on the form renderer or a schema-levelonChange. Repo-wide: no non-test source passes either (form_changeappears in nosrc/outside this renderer;plugin-form's Modal/Drawer/Split/Wizard/Tabbed forms, app-shell and console build form schemas withonDirtyChangeat most, and nothing forwardsonActionto the form renderer — matching #5235's "no in-repo caller currently does both"). Every test that pairs a form schema withonActionis inpackages/components(5 files); every test outside it that pairs a form schema withonChangeis inpackages/fields(4 files).plugin-formis included because it is the heaviest real driver of the reset path itself (record swaps, dirty guards). Callers passing inline arrows are bit-for-bit unaffected — their subscription was already torn down.Lint narrowing is a measurement, not a skip:
eslint.config.jsenables no type-aware linting (noparserOptions.project/projectService) and no custom rule ineslint-rules/reads another file, so a file's verdict is a function of its own text plus the shared config. My diff touches two files; both were linted, and the third piece — the before/after count on the same file — is above. The repo-wide run remains CI's.Generated by Claude Code