fix(Option): infer textValue from mixed children and keep value stable - #3076
Merged
Conversation
Contributor
Coverage Report for ./packages/components/
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Contributor
🚀 Preview DeploymentPreview environments are ready:
Images:
|
Contributor
✅ Visual Regression Tests PassedAll visual snapshots match the committed baselines. |
This was referenced Sep 2, 2026
An Option whose children were not a single text node lost both `textValue` and `value`, so react-aria assigned it a key off a render-order counter (`react-aria-1`). That key is what the field reports as its selected value and what `defaultValue` has to match, so the option submitted a meaningless string, could not be preselected, and shifted when unrelated markup around it changed — all silently. The Select Default story is the shape that hits it: `<Option>Millennium Falcon <Badge>Latest</Badge></Option>`. Inference now walks the children instead of requiring exactly one node. Only text that is a child itself counts: text inside an element child belongs to that element, so the option above is "Millennium Falcon", not "Millennium Falcon Latest". Remote text nodes (`RemoteTextRenderer`) count the same way, which keeps remote Options working. That walk is a new helper, `extractTextFromChildren`. `extractTextFromFirstChild` stays as it is — Button, Markdown, Initials and Truncate all depend on its "exactly one text node" strictness. `value` keeps defaulting to `textValue`; decoupling them would break every app relying on text-as-value. What does not survive is it silently becoming undefined: an Option with no inferable text now logs a console error naming the key fallback and asking for an explicit `value`. MIGRATION.md gains an entry — the key of a mixed-children Option changes from `react-aria-N` to its text, so anywhere that key was read back has to be checked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mfal
force-pushed
the
fix/option-text-value-3028
branch
from
September 2, 2026 11:53
1254222 to
e9acc62
Compare
mfal
marked this pull request as ready for review
September 3, 2026 12:26
This was referenced Sep 4, 2026
mfal
commented
Sep 4, 2026
mfal
commented
Sep 4, 2026
…-3028 # Conflicts: # packages/components/src/lib/react/remote.ts
#3053 landed `containsTextChild` with its own `isTextNode` predicate and its own fragment recursion, which is the same classification `joinTextChildren` does here. Two traversals, one piece of knowledge. `containsTextChild` now asks `extractTextFromChildren` and compares against `undefined`, so `isTextNode` goes away. One behavior change follows from the shared reading: `extractTextFromChildren` trims, so children that are only whitespace no longer count as text. That is the right answer for the question `Button` and `Link` ask — a blank string is not a label, and the control should keep its icon-only layout. Pinned by a test. The boolean answer no longer short-circuits at the first text child. Irrelevant at the child counts these components see. Also drops `Button` from `extractTextFromFirstChild`'s doc comment: #3053 replaced its call with `containsTextChild`, leaving `Markdown`, `Initials` and `Truncate` as the callers that depend on the strict reading. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mfal
enabled auto-merge (squash)
September 4, 2026 09:16
Lisa18289
approved these changes
Sep 4, 2026
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
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.
Root cause
Option.tsxdefaultedtextValue = extractTextFromFirstChild(children)andvalue = textValue.extractTextFromFirstChildbails out onChildren.count(children) !== 1, so text next to any element left bothundefined and react-aria assigned the item a key off a render-order counter.
That key is the option's form value and what
defaultValue/selectedKeyhasto match. So
<Option>Millennium Falcon <Badge>Latest</Badge></Option>— theSelect Default story — submitted
"react-aria-1", could not be preselected, andits key shifted when unrelated markup around it changed. Only the missing
textValuewarned; the key damage was silent.Decision 1 — inference walks the children
New helper
extractTextFromChildren(src/lib/react/remote.ts) joins the textamong the children instead of demanding exactly one node.
Only text that is a child itself counts. Text inside an element child
belongs to that element, so the option above is
"Millennium Falcon", not"Millennium Falcon Latest"— the Badge is decoration, not identity. Numberscount, fragments are transparent, and
RemoteTextRendererelements count thesame as strings, which is what keeps remote Options working.
extractTextFromFirstChildis untouched. Its four other callers all dependon the strict "exactly one text node" reading, and would break on a walk:
Buttondecides its icon-only layout by it,Markdowntakes it as the markdownsource,
InitialsandTruncateoperate on the whole content. Hence a newhelper rather than a changed one; both now carry JSDoc saying which question
they answer.
Decision 2 —
valuekeeps its default, but fails loudlyvaluestill defaults totextValue. Decoupling them is the bigger breakingchange: every app that relies on text-as-value would lose its keys at once, and
that default is a contract with extension developers.
What does not survive is
valuesilently becomingundefined. AnOptionwithno inferable text now logs
once per option, from an effect (so StrictMode does not double it). react-aria
already warns about the missing
textValuein that situation but says nothingabout the key, which is the expensive half.
Migration entry — yes, needed
packages/codemods/src/migrations/option-value-inferred-from-mixed-children/(
action: none,remotePackage: true). No prop changed shape, so this is achanged default, not a breaking prop change — but the selected key of an
existing app can change, from
react-aria-Nto the option's text. Anywherethat key was read back needs checking: a stored or server-side selection, a
defaultValuematched against it, a test asserting on it. Not mechanicallydecidable from source, so no codemod.
MIGRATION.mdandmigrations.generated.tsregenerated and committed;guide.test.ts's pinned newest-entry id updated.Verification
Test-first. Reverting
Option.tsxto the old inference fails 4 of the 6 newbrowser tests, with
expected [ 'react-aria-1', 'X-Wing', … ]— the issue'sexact symptom.
src/lib/react/remote.test.tsx— 20 unit tests over both helpers, includingthe remote-text and fragment paths
Option/Option.browser.test.tsx— pins thedata-keylist of the SelectDefault story shape, the emitted
onChangevalue,defaultValuetargeting,and the new console error
remote-react-components/src/tests/OptionKeys.browser.test.tsx— the samekeys in Local and Remote, the only guard for the remote-text path
Passed:
pnpm nx test:unit components(35 files / 285),pnpm nx test:compile components,pnpm nx test:browser components --browser.name=webkit(39 / 268),pnpm nx test:browser remote-react-components --browser.name=webkit(5 / 12),pnpm nx test:unit codemods(30 / 309),pnpm nx test:compile codemods,pnpm lint(0 errors),pnpm build(no further generated diff — props kepttheir shape, so
view.tsand theauto-generateddirs are unchanged).No rendered output changes, so no visual test was touched;
run-visual-testsisset to confirm that.
Decision 3 —
containsTextChildfolds into this walk#3053 merged meanwhile and added
containsTextChildto the same file, built onits own
isTextNodepredicate and its own fragment recursion — the sameclassification
joinTextChildrendoes here, traversed twice.It is now one line over the walk:
isTextNodeis gone. One behavior change comes with the shared reading:extractTextFromChildrentrims, so whitespace-only children stop counting astext. For the question
ButtonandLinkask — is this a labelled control oran icon-only one — that is the right answer, and a test pins it. The boolean no
longer short-circuits at the first text child, which does not matter at these
child counts.
extractTextFromFirstChild's doc comment losesButton: #3053 replaced itscall with
containsTextChild, soMarkdown,InitialsandTruncateare thecallers left that need the strict "exactly one text node" reading.
Separate commit (ba1b32e) on top of the merge, so the merge itself is a plain
union of both sides and the consolidation is reviewable on its own.
Still open: #3070 carries the same predicate as a local
isTextinNavigation.tsx. Left untouched here — it is Lisa's PR, andthe thread there
has the two-line swap.
fixes #3028
🤖 Generated with Claude Code