Skip to content
This repository was archived by the owner on Nov 4, 2026. It is now read-only.
This repository was archived by the owner on Nov 4, 2026. It is now read-only.

FormSchema.onChange is a declared public callback the form renderer never calls — authoring it is a silent no-op #4259

Closed

Description

@yinlianghui

Filed unassigned per Prime Directive #10, found while fixing #4247 (the gated-options cascade clear lives in the same effect region of form.tsx). Not in that card's scope.

What happens

FormSchema declares four lifecycle callbacks in @object-ui/types (packages/types/src/form.ts, interface FormSchema at line 1055):

onSubmit?: (data: Record"<"string, any">") =" ">" void | Promise"<"void">";
onChange?: (data: Record"<"string, any">") =" ">" void;
onDirtyChange?: (isDirty: boolean) =" ">" void;
onCancel?: () =" ">" void;

(generics spelled defensively here — the body sanitizer eats bare angle brackets)

The renderer destructures all four off schema together, in one block at packages/components/src/renderers/form/form.tsx:534-537:

onSubmit: onSubmitProp,
onChange: onChangeProp,
onDirtyChange: onDirtyChangeProp,
onCancel: onCancelProp,

Three of them are then wired. onChangeProp is never referenced again — the destructure is its only occurrence in the file:

$ grep -c onSubmitProp      form.tsx   -> 3
$ grep -c onDirtyChangeProp form.tsx   -> 5
$ grep -c onCancelProp      form.tsx   -> 4
$ grep -c onChangeProp      form.tsx   -> 1     (the destructure itself)

So a consumer that wires onChange on a form schema is handed a typed, documented, autocompleted API that does nothing at all. There is no warning, no dev-mode notice, and no type error — the declaration says it is supported.

Why it is worth a card rather than a deletion in passing

Two readings, and they need a maintainer rather than a guess:

  • Wire it — the value-change channel is the one lifecycle hook missing from a form that already publishes submit / dirty / cancel, and onDirtyChange right beside it proves the subscription plumbing (a form.watch equivalent) already exists here. If any consumer wants live values, this is the declared place for them.
  • Retire it — declared-but-unenforced is the enforce-or-remove family (ADR-0049 in the sibling repo): if nothing pulls on it, the honest fix is removing the declaration so it stops advertising a capability that is not there.

Which one is right depends on whether there is real business pull for a live value channel on FormSchema, which is a product call. Note the two are not symmetric in cost: wiring it is additive and safe, while removing it is a public type change on @object-ui/types.

Reachability

Any consumer authoring onChange on a form schema — the prop is public, exported and documented. I did not find an in-repo caller, which is why this reads as dormant drift rather than a live break; a downstream consumer of @object-ui/types cannot be checked from here.


Generated by Claude Code

Activity

  1. os-zhuang commented on Aug 11, 2026

    @os-zhuang
    Contributor

    Findings triage (first grade): HOLD as finding.

    • Premise verified on origin/main @ b954120: onChangeProp occurs exactly once in packages/components/src/renderers/form/form.tsx (the destructure) — the declared callback is never invoked, while its three siblings are wired. Declared≠enforced, one prop wide.
    • Why hold: dormant — no in-repo caller authors onChange on a form schema; nothing breaks today. The card's own framing is right that the exit (wire it vs retire the declaration) is a product call with asymmetric costs (additive wiring vs a public type change on @object-ui/types).
    • Promotion shape: when graded up, this goes to the decision box, not the queue — the fork needs the maintainer. Holding rather than escalating now because the box is at capacity (~23 objectstack + 3 objectui open decisions) and a dormant no-op loses nothing by waiting; the first downstream consumer report of a dead onChange flips this to promote-immediately.
    • Dedup: no open sibling on FormSchema lifecycle callbacks; the same-session A dependency-gated option list clears the field's stored value on mount, before the user touches anything #4247 is the cascade-clear defect in the same effect region, correctly split.

    本评论来自分诊座位 Routine(#5474 试点),不构成认领。


    Generated by Claude Code

  2. added theissue type on Aug 17, 2026
  3. os-zhuang commented on Aug 17, 2026

    @os-zhuang
    Contributor

    Concentrate round: finding → needs-user-decision, type Bug — executing the standing verdict's own promotion shape. The 2026-08-11 grading ruled "when graded up, this goes to the decision box, not the queue" and held only because the box was at capacity; the maintainer cleared the box today, so the capacity rationale is gone and the already-decided shape executes.

    The fork (unchanged): FormSchema.onChange is a published, typed, documented callback the form renderer never invokes — wire it or retire it. Retiring deletes a published capability from @object-ui/types (maintainer-floor by standing rule); wiring is additive.

    Four-prism:

    • Platform coherence — a declared lifecycle callback must fire or not exist; three wired siblings beside one inert one is the incoherence.
    • Measured business pull — zero in-repo callers; downstream consumers unverifiable from here. Nobody is known to be waiting on either exit.
    • AI-agent error-resistance — a typed, autocompleted no-op is a trap tailor-made for agents: they will author it and trust it. Either exit removes the trap; leaving it declared-and-dead is the only wrong answer.
    • Startup scope discipline — retiring is remove-over-maintain but costs a breaking public type change; wiring is small (the onDirtyChange subscription plumbing sits beside it) but activates a capability with zero demonstrated pull.

    Recommendation: wire it — small, additive, makes the declaration true with existing plumbing; retirement stays available at the next breaking window if the maintainer prefers shrinking the callback surface. Decide once for the declaration; no third state.

    This comment is from the triage seat Routine — session session_01Y6RnGGr6eNirCHrU7QmFRd.


    Generated by Claude Code

  4. os-zhuang commented on Aug 17, 2026

    @os-zhuang
    Contributor

    Maintainer ruling recorded — maintainer, 2026-08-17, batch acceptance of the PM decision-inbox recommendations, verbatim: 「同意」. Four-dimension analysis delivered in-session; provenance: PM session session_01NYgmGheCzM6NrHZN436Cxf.

    Ruled: wire it. FormSchema.onChange gets called with live form values, using the same subscription plumbing onDirtyChange proves exists in the file. Enforce-or-remove is satisfied on the enforce side because it is the strictly cheaper arm here: wiring is additive; removal is a breaking public-type change plus a deprecation cycle for a hook whose siblings all work. Pin test: authored onChange fires on value change; absence of onChange changes nothing.

    Label needs-user-decision → pm:queue in the same stroke.


    Generated by Claude Code

  5. os-support-ai commented on Aug 18, 2026

    @os-support-ai
    Collaborator

    Claim: PM loop round 6
    Session: session_01RV6yuVCxymHYE16PL9vQkE
    Branch: claude/issue-4259-formschema-onchange-wiring
    Worktree: objectui-issue-4259
    Domain: repo:objectui (execution seat)
    File surface: packages/components/src/renderers/form/form.tsx plus a new test under packages/components/src/renderers/form/__tests__/. ⛔ OUT: packages/types/** (see Zone 2 — the declaration is already correct, and #5186 is in flight on packages/types/src/zod/form.zod.ts), packages/app-shell/src/views/metadata-admin/** (#5170+#5169, PR5226), packages/plugin-view / plugin-list (#5177), packages/plugin-grid + packages/core (#5217), packages/plugin-dashboard (#5064 parked on a decision).
    Container & model: S, mode:subagent, model: opus — the edit is a handful of lines, but choosing the effect phase and proving the negative half of the pin is judgement. Tier gate run this round on the actual paths: "Model tier — no path-derived mandate: the surface hits none of the 1 declared glob(s) … The tier stays the PM's per-card judgment call (floor sonnet · default opus · ceiling fable)."
    Clause-②: no — the callback is already declared in @object-ui/types. Making the renderer honour a published declaration does not widen the public surface and does not move any accept/reject boundary.
    Serial constraints cleared: packages/components/src/renderers/form/form.tsx is free — #3439 merged 14:00Z and PR5219 merged; none of the five in-flight cards declares it, and I diffed all four live branches against origin/main to confirm (5170 touches app-shell/views/metadata-admin/** only; 5177/5186/5217 have no commits yet and their declared surfaces are elsewhere). No read-coupling: no in-flight card pins onChange behaviour.

    Zone 1 — the ruling is binding, and it already chose the exit

    Maintainer, 2026-08-17, batch acceptance of the PM decision-inbox recommendations, verbatim: 「同意」. Recorded on this card:

    > Ruled: wire it. FormSchema.onChange gets called with live form values, using the same subscription plumbing onDirtyChange proves exists in the file. Enforce-or-remove is satisfied on the enforce side because it is the strictly cheaper arm here: wiring is additive; removal is a breaking public-type change plus a deprecation cycle for a hook whose siblings all work. Pin test: authored onChange fires on value change; absence of onChange changes nothing.

    ⛔ The wire-vs-retire fork is closed. Do not re-open it, do not propose deprecating the declaration, do not add a dev-mode warning as a substitute. If implementation turns up a fact that makes wiring wrong, that is the 回翻 case: stop, do not decide it yourself, report it and the card comes back to the decision box.

    Premise — re-verified against origin/main today, and the line anchors have MOVED

    The card body's anchors are from 2026-08-11 and are stale. Measured this round:

    • destructure block is at 995–998, not 534–537
    • onChangeProp occurs exactly once in the file — line 996, the destructure itself. Still a silent no-op. Counter-probed with onDirtyChangeProp, which returns 5 hits, so the single hit is a real reading and not a broken pattern.
    • siblings wired: onSubmitProp at 1694–1695, onCancelProp at 1772–1773, onDirtyChangeProp at 1527 / 1562 / 1567.

    Take every anchor from the tree, not from this comment either. Five cards this round had bodies that had drifted.

    The plumbing — there are TWO patterns in this file and they are not interchangeable

    Both sit within 40 lines of each other:

    1. The onAction subscription (~1541): React.useLayoutEffect with an if (onAction) guard around form.watch(...), returning subscription.unsubscribe().
    2. The onDirtyChange subscription (~1560): React.useLayoutEffect, subscribes unconditionally, calls onDirtyChangeProp?.(...) inside.

    Pattern 1 is the one that matches the ruling's second half. "absence of onChange changes nothing" is only literally true if no subscription is created when onChangeProp is absent — an unconditional third form.watch on every form in the product is a behaviour change for callers who authored nothing. Guard it.

    Layout phase is not decoration. The comment above the onAction effect says exactly why it is useLayoutEffect and not useEffect: React runs every layout destroy before any layout create, so a caller passing a fresh callback each render (the common inline-arrow case) has the subscription torn down before the defaultValues reset runs and re-established after — "That is what keeps a reset from being reported as a user edit." A passive effect inverts the order and a record landing looks like the user editing every field it filled. Read that comment in full before you write the effect. If you deviate from layout phase, the PR body has to say why.

    Zone 2 — scope boundaries, stated because two of them are easy to trip

    1. packages/types is out. The declaration onChange?: (data) =&gt; void is already there and already correct — this card is the renderer failing to honour it. If you find yourself editing the type, you have left the card.
    2. The top-level strip at ~2425 is a trap worth reading, not a second fix. The props destructure drops onChange: _ignoredOnChange with the comment "Prevent overwriting our onChange" — and today there is no "our onChange" to overwrite; the callback the comment is protecting does not exist. After your change that comment becomes true for the first time. You may correct that comment so it names what it is actually protecting (a DOM onChange on the form element would fire with a SyntheticEvent, not with form values). ⛔ Do not change what the strip does — no double-fire, no re-routing the top-level copy. Behaviour of that block stays byte-identical.

    Tests — the pin has two halves and the second one is the one that gets skipped

    The ruling names both. Deliver both:

    • Positive: an authored onChange fires on a value change, and receives the form values, not an event. Assert on a value, not just on call count.
    • Negative: absence of onChange changes nothing. This is the half that goes missing. Make it observable — a form with no onChange behaves identically, and in particular no extra subscription is established.
    • Reset ordering: a defaultValues reset is not reported to onChange as a user edit. This is the property the layout phase exists to protect; if you get it right by copying the pattern, pin it, or the next refactor takes it away silently.

    Reverse-verify: restore form.tsx to origin/main, keep the new test file, predict which assertions go red before running, and report predicted-vs-observed. An assertion that stays green pre-fix is telling you it pins nothing.

    Gates

    Derive from the actual diff — expect Lint, Type Check, the shards, changeset gates, Control Byte Scan. Note scripts/pm/dispatch-gates.mjs is objectstack-only; in this repo derive the gate set from .github/workflows and package.json. Changeset required (@object-ui/components); score patch or minor, ⛔ never major — the 39 packages share one fixed version group and a major versions the whole fleet.

    ⛔ Do not background a long check and end your turn — three devs hit that trap today. If a suite queues behind the shared verify lock, narrow it and declare the narrowing in the report.

    Deliver

    Draft PR against main, first body line Fixes #4259. Push the branch early. Return the JSON report and post it as an issue comment whose first line is the literal &lt;!-- os-dev-report --&gt; marker.


    Generated by Claude Code

  6. os-support-ai commented on Aug 18, 2026

    @os-support-ai
    Collaborator
    {
      "issue": 4259,
      "status": "done",
      "branch": "claude/issue-4259-formschema-onchange-wiring",
      "pr": "https://github.com/objectstack-ai/objectui/pull/5238",
      "premise_still_valid": true,
      "summary": "Wired FormSchema.onChange in packages/components/src/renderers/form/form.tsx: a new form.watch subscription beside its two siblings, calling onChangeProp with the live form values. It is GUARDED on onChangeProp (the onAction pattern, not the unconditional onDirtyChange one) so a schema authoring no onChange establishes no subscription at all, which is what makes the ruling's 'absence of onChange changes nothing' literally true rather than merely approximately true. It runs in the LAYOUT phase like the defaultValues reset and both subscriptions beside it, so a caller passing a fresh inline arrow has it torn down around the reset and a landing record is not reported as a user edit. Also corrected the now-stale comment on the top-level onChange props strip, which claimed to protect an 'our onChange' that did not exist until this change; that block's behaviour is byte-identical. packages/types untouched — the declaration was already correct.",
      "tests": "All gates run against the final commit ec223d150 (tree clean, HEAD == pushed, git diff HEAD empty). NEW FILE packages/components/src/renderers/form/__tests__/form-onchange-wiring.test.tsx, 4 tests: positive (asserts on a VALUE — name:'Alice' then 'Bob' — and that it is not an event: no nativeEvent/target/preventDefault); negative made OBSERVABLE (a probe wraps the useForm result and counts form.watch(callback) calls: authoring onChange costs exactly +1 subscription, authoring nothing costs 0); behavioural equivalence without onChange; reset-ordering with a liveness tail. RUNS: `pnpm exec vitest run packages/components/` -> 'Test Files 161 passed (161) / Tests 1468 passed (1468)'. `pnpm --filter @object-ui/components run type-check` -> exit 0, script name echoed in output so it is not the zero-match trap. eslint on both changed files -> 0 errors (54 pre-existing warnings). check:control-bytes -> 'OK (scanned 4642 tracked text file(s))'. check:phantom-deps, check:self-import, check-changeset-presence ('1 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)'), check-changeset-no-major -> all pass. DECLARED NARROWING: local test scope is packages/components/ (the changed package), not the whole farm; CI runs the farm once anyway. Dependency closure built first via `pnpm --workspace-concurrency=2 --filter '@object-ui/components^...' build` — without it type-check fails on unbuilt siblings (@object-ui/i18n, sdui-parser, data-objectstack) in a way that reads exactly like a real error. REVERSE VERIFICATION (fix committed first, then form.tsx restored to origin/main, test file kept; predictions stated before running): predicted 3 RED / 1 GREEN, observed exactly that — 'expected 0 to be greater than 0'; 'expected 1 to be 2'; test 3 GREEN; 'expected undefined to deeply equal ObjectContaining {name: Edited}'. HONEST CAVEAT: two assertions stay green pre-fix and pin nothing about this fix — test 3 entirely (it is a regression guard for the negative half), and the ORDERING assertion inside test 4 is VACUOUSLY green pre-fix (onChange never fires, so 'the reset is not among the calls' is trivially true); that is precisely why the liveness tail was appended, and the tail is the assertion that actually goes red. expect(withoutOnChange).toBe(1) in test 2 is likewise a deliberate ratchet on pre-existing behaviour, not a pin on this change. form.tsx restored from the commit afterwards, byte-identical.",
      "open_questions": [],
      "out_of_scope_findings": [
        "filed as #5235: the 'a defaultValues reset is not a user edit' guarantee is delivered by the callback's IDENTITY changing each render, so a caller who memoizes with useCallback keeps a live subscription across the reset and does receive it — measured, a stable-identity onChange gets {name:'Loaded',note:'from server'} while an inline arrow gets nothing. Pre-existing on the onAction channel; onChange inherits it by following the ruled plumbing. Filed unassigned, no pm:queue label — picking the right semantics is a design question, not a mechanical fix, so it was not decided here."
      ]
    }

    Changeset: .changeset/formschema-onchange-wiring-4259.md, @object-ui/components: minor (never major — 39 packages share one fixed version group). No skip-changeset label involved: that mechanism does not exist in this repo, and this PR carries a real changeset regardless.

    Report delivered at draft-PR time per the standing clause; CI convergence is the PM's to read.


    Generated by Claude Code


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions