Skip to content

fix(components): keep field values controlled from the first render - #3078

Open
mfal wants to merge 7 commits into
mainfrom
fix/modal-uncontrolled-to-controlled-3026
Open

fix(components): keep field values controlled from the first render#3078
mfal wants to merge 7 commits into
mainfrom
fix/modal-uncontrolled-to-controlled-3026

Conversation

@mfal

@mfal mfal commented Sep 2, 2026

Copy link
Copy Markdown
Member

Every Flow field component that renders without value and without defaultValue flipped from uncontrolled to controlled on its first change. This is what produced the nine WARN: A component changed from uncontrolled to controlled. warnings reported from Modal.browser.test.tsx.

The defect

useControlledHostValueProps (packages/components/src/lib/remote/useControlledHostValueProps.ts) mirrors a field's value in state and hands that state to react-aria. Its onChange writes the state unconditionally, so the hook owns the value from the first change on — but the state started as regularValue ?? defaultValue, i.e. undefined for an uncontrolled field.

react-aria's useControlledState reads only undefined as uncontrolled. So the field got undefined on render one and a real value on the first keystroke: it warns about the transition, and the value silently changes owner mid-flight, from the DOM input to the hook.

Eight components run this hook: TextField, TextArea, SearchField, NumberField, PasswordCreationField, MarkdownEditor, CodeEditor, DateRangePicker. Seven of them warn. CodeEditor does not, because CodeMirror keeps its own document state and never calls useControlledState — it is affected by the same ownership change, just silently.

Modal is not involved — #3026's premise is disproven

#3026 read the warnings as Modal's own isOpen flipping between its prop path and its controller path. It does not, and there is no separate Modal defect: Overlay.tsx:68 resolves isOpen as isOpenFromProps ?? controller.useIsOpen(), OverlayController.isOpen is initialised false (OverlayController.ts:54), and useIsOpen() returns it through useSelector. So isOpen is a defined boolean on every render, on both paths — it never crosses the line.

Measured with console.warn instrumented to print the emitting test plus a stack:

  • <Modal isOpen>, then a rerender to isOpen={false}: 0 warnings.
  • <Modal controller={…}> opened via the controller: 0 warnings.
  • <ModalTrigger><Modal> opened by its button: 0 warnings.
  • A Form + Field name="x" + TextField, no modal anywhere: 1 warning.
  • A bare <TextField /> typed into, no form, no modal: 1 warning.

All nine warnings in Modal.browser.test.tsx come from the TextField inside a Field without a defaultValue — they share #3027's root cause, and they are gone with this fix. The four Modal tests that pass defaultValue="" never emitted any. #3026 is therefore closed by this PR as the symptom it reported, not as a defect of its own.

The judgement call

Which side of the line the fields land on. Controlled from the first render.

The hook already makes every field controlled from the first change on — that is its purpose, protecting a remotely driven value from marker values that must be ignored. Starting controlled only extends what already holds for the rest of the field's life, and it is the smaller change: react-aria renders the input from useControlledState's current value either way, so nothing about the rendered output moves.

The alternative — stay uncontrolled until a value actually arrives — hands ownership back to react-aria. That is architecturally cleaner but changes far more (external resets and defaultValue start flowing through react-aria again), and it breaks three consumers outright, all of which read the mirrored value themselves:

  • MarkdownEditor renders it in preview mode (MarkdownEditor.tsx:157) and feeds it to modifyValueByMarkdownSyntax for the toolbar actions.
  • CodeEditor feeds it to its CopyButton (CodeEditor.tsx:199).
  • PasswordCreationField validates it against the password policy — usePolicyValidationResult(validationPolicy, value ?? "", …) (:131) and isEmptyValue = !value (:162). Without a defined mirror, the strength meter and the policy errors go dead for every uncontrolled password field.

Being controlled from render one needs a defined empty value, which is per type and cannot be derived generically. The hook now takes it as an argument, matching what react-aria's own state uses for "nothing entered": "" for the text inputs, NaN for NumberField (useNumberFieldState), null for DateRangePicker (useDateRangePickerState). Because those are react-aria's own uncontrolled fallbacks, the rendered output is identical either way for all three types.

The fallback is on undefined only, not ??: null is how a caller controls a DateRangePicker with no range selected, and ?? let it fall through to defaultValue for one commit.

Verification

  • pnpm nx test:browser components --browser.name=webkit — 39 files, 280 tests pass. Zero uncontrolled to controlled warnings remain in Modal.browser.test.tsx.
  • pnpm nx test:browser remote-react-components --browser.name=webkit — 5 files, 18 tests pass.
  • pnpm nx test:unit components — 253 pass. pnpm nx test:compile components — clean. pnpm lint — 0 errors.
  • pnpm build over the workspace leaves git status clean: no @flr-generate component's props changed, so there is nothing to regenerate.

Tests, all red before the fix

  • TextField stays controlled across the first change pins the root cause; Modal and its fields stay on one side of the controlled line pins Modal flips isOpen from uncontrolled to controlled between its prop and controller paths #3026's reproduction.
  • useControlledHostValueProps.browser.test.tsx drives all eight fields, uncontrolled and controlled, and fails on uncontrolled to controlled from react-stately or React DOM. 7 of 8 were red per browser before the fix, which is how the CodeEditor detail above was established.
  • RemoteControlledValue.browser.test.tsx covers the reason the hook exists at all. The remote echo path had no test whatsoever: with the marker filter removed, the literal string ___flowControlledRemoteValue___ reaches the host input and the entire existing browser suite stays green. The new file asserts, in both the Local and Remote environments, that everything typed survives, that a value the remote app sets itself still arrives (the non-marker path must not be swallowed), and that neither a controlled nor an uncontrolled remote field crosses the line. Removing the marker filter fails three of its four tests.

One AGENTS.md row

The compile fix in aaee11a79 exists because test:compile was run for components but not for remote-react-components, where the new test file lives — and that package's browser tests passing said nothing about its types, since vitest transpiles without typechecking. So the only gate that reads types there is exactly the one a per-package run skips, and the error surfaced in CI.

ec5bda925 adds that as one row to the Common failures table, in its own commit so it reads separately from the fix. Nothing else in AGENTS.md.

Deliberately out of scope

fixes #3027
fixes #3026

🤖 Generated with Claude Code

@mfal mfal self-assigned this Sep 2, 2026
@mfal mfal added the run-visual-tests Runs the full visual regression suite against the existing baselines and fails the check on mismatch label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Coverage Report for ./packages/components/

Status Category Percentage Covered / Total
🔵 Lines 78.69% 746 / 948
🔵 Statements 78.57% 763 / 971
🔵 Functions 80.09% 165 / 206
🔵 Branches 70.33% 377 / 536
File CoverageNo changed files found.
Generated in workflow #6676 for commit 29ec4bb by the Vitest Coverage Report Action

@github-actions github-actions Bot removed the run-visual-tests Runs the full visual regression suite against the existing baselines and fails the check on mismatch label Sep 2, 2026
@mfal

mfal commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Dispatched independently on #3027 (useControlledHostValueProps flips all eight fields), I reached the same decision as this PR — controlled from the first render, with a per-type empty value passed into the hook ("" / NaN / null). Same reasoning, arrived at separately, so no competing PR. Everything below is either a verification of a claim in the body, or something to add.

Verified independently

  • MarkdownEditor / CodeEditor need a defined mirror. Correct, and there is a third: PasswordCreationField feeds the mirror into usePolicyValidationResult(validationPolicy, value ?? "", …) and derives isEmptyValue = !value from it (PasswordCreationField.tsx:131, :162). Omitting value while uncontrolled would leave the strength meter and the policy errors dead for every uncontrolled password field. The argument for this direction is stronger than the body states.
  • Modal's isOpen never crosses the line. Overlay.tsx:68 resolves isOpenFromProps ?? controller.useIsOpen(); OverlayController.isOpen is initialised false (OverlayController.ts:54) and useIsOpen() returns it through useSelector, so it is a boolean on every render on both paths.
  • NaN and null are react-aria's own empty values. useNumberFieldState calls useControlledState(value, isNaN(defaultValue) ? NaN : defaultValue, onChange); useDateRangePickerState calls useControlledState(props.value, props.defaultValue || null, props.onChange). So the rendered output is identical to the uncontrolled fallback in both cases — the "nothing moves visually" claim holds for the two non-string fields too, which are the risky ones.
  • No regeneration needed. pnpm build over the whole workspace leaves git status clean — no @flr-generate prop changed.

One nit in the diff

useState(regularValue ?? defaultValue ?? emptyValue) uses ??, so a null value falls through. That matters only for DateRangePicker, where value={null} is react-aria's documented "controlled, nothing selected": a caller passing value={null} and a defaultValue renders the defaultValue for one commit before the layout effect below corrects it to null. Contradictory props, so low stakes — but the explicit form costs nothing:

const [value, setValue] = useState(
  regularValue !== undefined
    ? regularValue
    : defaultValue !== undefined
      ? defaultValue
      : emptyValue,
);

The marker cannot appear on the first render (it is only sent once a remote event has been handled), so regularValue there is always the caller's own value.

Missing: coverage of the remote path, and of the other seven fields

This PR tests TextField plus the Modal reproduction. Two gaps:

  1. The remote echo filter is untested. The hook lives in lib/remote/ because the mirror is what makes the host ignore the echo of its own keystrokes (controlledRemoteValueMarker). Nothing in the suite crosses that path, so a future change to the mirror can break it silently. I measured how silent: with the marker filter removed, the literal string ___flowControlledRemoteValue___ lands in the host input and the whole existing browser suite stays green.
  2. Seven of the eight fields are untested for the flip, including the two carrying NaN and null.

Both are on fix/controlled-host-value-props-3027, and commit 55e715a54 is just the two test files — cherry-pick it onto this branch and it applies on top of your fix unchanged (my f967a05d2 differs from your hook change only in the nit above):

  • packages/components/src/lib/remote/useControlledHostValueProps.browser.test.tsx — drives all eight fields, uncontrolled and controlled, and fails on uncontrolled to controlled from react-stately or React DOM. Before the fix: 7 of 8 red per browser (CodeEditor stays green — CodeMirror does not use useControlledState).
  • packages/remote-react-components/src/tests/RemoteControlledValue.browser.test.tsxLocal and Remote: everything typed survives, a value the remote app sets itself still arrives (the non-marker path must not be swallowed), and neither a controlled nor an uncontrolled remote field crosses the line. The last one is red before the fix in both environments; removing the marker filter fails three of the four.

One finding from writing that file, worth knowing: in the Remote environment the remote side does not see every keystroke — a host event that fires while a remote render is in flight is dropped, load-dependently (firefox drops ~half of 19 keystrokes, webkit none). Pre-existing, unrelated to this change, and it makes "the remote side received the full text" an unusable assertion; the test asserts every reported value is a prefix of what was typed instead.

Issue numbers

The body proves Modal's isOpen never flips, and the code shipped here is the #3027 fix. So the trailer should read fixes #3027, with #3026 closed as not-a-defect referring here. Left as fixes #3026, #3027 stays open on top of a merged fix.

Also worth the run-visual-tests label: eight components change which side of the controlled line they start on. Nothing should move (see the react-aria fallbacks above), and the label is verify-only.

Recommendation: merge this one, with the nit and the two test files folded in.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🚀 Preview Deployment

Preview environments are ready:

Type URL
docs pr-3078.docs.review.flow-components.de
storybook pr-3078.storybook.review.flow-components.de

Images:

  • docs: ghcr.io/mittwald/flow/docs:pr-3078
  • storybook: ghcr.io/mittwald/flow/storybook:pr-3078

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

✅ Visual Regression Tests Passed

All visual snapshots match the committed baselines.

Run details

@mfal mfal added the run-visual-tests Runs the full visual regression suite against the existing baselines and fails the check on mismatch label Sep 2, 2026
@mfal

mfal commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Landed everything from the review above onto this branch, so nothing needs cherry-picking any more:

  • f9e956eb5 — the two test files (all eight fields; the remote echo path).
  • 19e875cc9 — the ?? nit: fall back on undefined only, so a controlled value={null} cannot slip into the defaultValue fallback for a commit.

Both test suites coexist — the cherry-pick did not touch TextField.browser.test.tsx or Modal.browser.test.tsx.

Gates on the merged result: test:unit components 253 pass · test:compile components clean · test:browser components --browser.name=webkit 39 files / 280 tests pass · test:browser remote-react-components --browser.name=webkit 5 files / 18 tests pass · pnpm lint 0 errors · pnpm build leaves git status clean.

Body updated: both fixes #3027 and fixes #3026 (with the paragraph disproving #3026's premise, so it can be closed knowingly), the PasswordCreationField finding, the CodeEditor detail, and the untested-echo-path finding. run-visual-tests added — verify-only.

The dropped-keystroke observation from the review is now #3088, out of scope here and referenced from the body.

@github-actions github-actions Bot removed the run-visual-tests Runs the full visual regression suite against the existing baselines and fails the check on mismatch label Sep 2, 2026
mfal and others added 5 commits September 2, 2026 13:51
`useControlledHostValueProps` owns a field's value from the first change on,
but started its mirror state as `undefined` when the field rendered with
neither `value` nor `defaultValue`. react-aria then got `undefined` first and a
real value on the first keystroke: `useControlledState` reads only `undefined`
as uncontrolled, warns about the transition, and the value changes owner
mid-flight from the DOM input to the hook.

The hook now takes the field's empty value and is controlled from render one —
`""` for the text inputs, `NaN` for `NumberField`, `null` for
`DateRangePicker`, matching what react-aria's own state uses for "nothing
entered". Behaviour after the first change is unchanged; the input was already
rendered from `useControlledState` either way.

This is what produced the nine `uncontrolled to controlled` warnings reported
against `Modal` — every one came from the `TextField` inside a `Field` without
a `defaultValue`, not from the modal's open state. Across the browser suite the
warnings drop from 20 to 4: 2 are the `Tabs` defect fixed in #3025, 2 are
`Field` feeding `selectedKey` to `Select`, which needs its own decision about
overriding `defaultSelectedKey`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two files, both red before the fix in
`useControlledHostValueProps`:

- `useControlledHostValueProps.browser.test.tsx` drives all eight fields
  that run the hook, uncontrolled and controlled, and fails on any
  `uncontrolled to controlled` message from react-stately or React DOM.
  Seven of the eight warned before the fix — `CodeEditor` did not, because
  CodeMirror does not use `useControlledState`.
- `RemoteControlledValue.browser.test.tsx` covers the reason the hook
  exists. The mirror is what makes the host ignore the echo of its own
  keystrokes (`controlledRemoteValueMarker`), so a change to the mirror is
  a change to that protection. Asserted in both environments: everything
  typed survives, a value the remote app sets itself still arrives, and
  neither a controlled nor an uncontrolled remote field crosses the line.
  Dropping the marker filter puts the literal marker into the host input,
  which fails three of the four tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ue fallback

`regularValue ?? defaultValue ?? emptyValue` falls back on `null` as well
as on `undefined`, but only `undefined` means uncontrolled to react-aria.
`null` is how a caller controls a `DateRangePicker` with no range
selected, so a caller passing `value={null}` together with a
`defaultValue` rendered the `defaultValue` for one commit, until the
layout effect below replaced it with `null`.

Fall back on `undefined` only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…g its element

`Locator.element()` is typed `HTMLElement | SVGElement`, so reading
`.value` off it fails `test:compile` with TS2339 — the package's own
`test:compile`, which is not covered by running the one in `components`.

Assert through `expect.element(locator).toHaveDisplayValue(…)` instead of
`expect.poll(() => locator.element().value)`. That is what the other
browser tests use for an input's value (`TextField`, `MarkdownEditor`,
`Form`, `ResetButton`), it needs no cast, and it keeps the retrying
behaviour the `Remote` environment depends on.

All four call sites resolve the same `input` locator. What they assert is
unchanged: with the marker filter removed, the literal
`___flowControlledRemoteValue___` still reaches the host input and three
of the four tests still fail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A diff that touches a second package leaves that package's
`test:compile` unrun, and its browser tests passing says nothing about
types — vitest transpiles without typechecking. So the only gate that
reads types there is the one a per-package run skips, and the error
surfaces in CI instead. Cost that exact round trip on this PR.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mfal
mfal requested a review from ins0 September 3, 2026 12:13
@mfal
mfal marked this pull request as ready for review September 3, 2026 12:13
@mfal
mfal requested a review from a team September 3, 2026 12:13
mfal added a commit that referenced this pull request Sep 4, 2026
The rationale for `?? null` was a 14-line block comment in `Tabs.tsx`, and
#3078 carries the same react-aria fact again in
`useControlledHostValueProps`. Two copies in two files that are never read
together.

Move it to one bullet in the package's non-obvious conventions: react-aria's
`useControlledState` reads only `undefined` as uncontrolled, so a component
that mirrors the value in its own state has to pass a sentinel. The bullet
covers both sentinel shapes (`null` for "nothing selected yet", the type's
empty value otherwise), the `??` trap once the sentinel is `null`, and the
silent case (`CodeEditor` changes owner without warning).

The comment at the call site keeps what a reader needs there: which value
keeps it controlled, a pointer to the convention, and why the cast exists.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The hook explained react-aria's controlled/uncontrolled rule in full, and
#3048 explained the same rule again in `Tabs.tsx`. That rationale now lives
in one bullet in the package's AGENTS.md (added by #3048), so both call sites
can stop carrying a copy.

What stays here is what a reader needs at this spot: what `emptyValue` is per
field type, and why the initialisation spells out `!== undefined` instead of
`??` — a caller-supplied `null` controls a `DateRangePicker` with no range
selected and must not fall through to `defaultValue`.

Comment-only; no behaviour change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant