refactor(value-editor): migrate to TypeScript and split by concern - #8447
Draft
talissoncosta wants to merge 10 commits into
Draft
refactor(value-editor): migrate to TypeScript and split by concern#8447talissoncosta wants to merge 10 commits into
talissoncosta wants to merge 10 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
talissoncosta
force-pushed
the
refactor/value-editor-typescript
branch
from
September 2, 2026 18:46
9930bc1 to
61e17f1
Compare
talissoncosta
force-pushed
the
refactor/value-editor-typescript
branch
from
September 2, 2026 21:12
b129ad3 to
7f4289b
Compare
talissoncosta
force-pushed
the
refactor/value-editor-typescript
branch
from
September 2, 2026 21:19
7f4289b to
1a381c2
Compare
talissoncosta
force-pushed
the
refactor/value-editor-typescript
branch
from
September 2, 2026 22:09
4e0edeb to
d0fef50
Compare
4 tasks
talissoncosta
force-pushed
the
feat/input-placeholder-and-value-copy-8441
branch
from
September 4, 2026 11:52
ca926e8 to
e86fc77
Compare
talissoncosta
force-pushed
the
refactor/value-editor-typescript
branch
from
September 4, 2026 11:52
d0fef50 to
0d08e41
Compare
talissoncosta
force-pushed
the
feat/input-placeholder-and-value-copy-8441
branch
3 times, most recently
from
September 4, 2026 12:58
201b683 to
f9a428e
Compare
Base automatically changed from
feat/input-placeholder-and-value-copy-8441
to
main
September 4, 2026 13:41
ValueEditor was a 254-line untyped class with no props contract, which is
how `placeholder` and `readOnly` came to be passed at three call sites and
silently ignored. Renaming the file to .tsx produced 43 strict errors; all
but two were missing interfaces and implicit any.
The two that were not:
- renderValidation() was called with an argument it does not accept
- <Tooltip position='top'> passes a prop TooltipProps does not declare
Both are gone. `value` is typed FlagsmithValue rather than string, which is
what the MV and segment call sites actually pass.
Split into a folder by concern, so each piece is small and the parsing is
testable on its own for the first time:
ValueEditor/
ValueEditor.tsx orchestrates, function component
types.ts language union and display order
validate.ts pure parse checks, no React
components/LanguageSelector the .txt/.json/.xml/.toml/.yaml row
components/LanguageValidation the tick or warning
components/CopyValueButton the copy control
index.ts
Imports are unchanged: the folder resolves where the file used to.
Drop ionicons from this component. We already own `warning` and
`checkmark-circle`, so it is a straight swap. Icon forwards className for
only 12 of its ~100 cases and `warning` hardcodes orange, so the tone class
goes on a wrapper and the icon inherits through currentColor.
Copying no longer goes through polyfill-react-native's Clipboard, which
wraps the deprecated document.execCommand. Extract copyToClipboard out of
the Utils object into common/utils/copyToClipboard, so a component can
reach it without pulling in Utils; Utils keeps the key and delegates, so
its nineteen existing callers are untouched. The Storybook stub for Utils
asks new components not to import it, and this is what that looks like.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Chrome puts <parsererror> in the XHTML namespace, so it is an HTMLElement with innerText and the old instanceof check held. Firefox uses its own namespace and returns a plain Element with neither, so malformed XML reported no error and the editor showed a green tick. Our E2E runs on Firefox. Reading textContent off Element works in both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
validate.ts and copyToClipboard.ts are plain .ts with no React, which is the only thing this project can unit test today: jest runs with testEnvironment 'node' and testMatch picks up .ts but not .tsx, and there is no React Testing Library. The xml branch is left as it.todo. It needs DOMParser, which the node environment does not provide, and pulling in jest-environment-jsdom is a wider decision than this file. Writing these caught a wrong assumption: 'a:\n- b\n - c' parses fine, so the yaml failure case now uses an unclosed quote and a bad indent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The .txt/.json/.xml/.toml/.yaml labels were <span onMouseDown>: not focusable, no role, no keyboard path. That is the same defect the copy control had, and leaving four of them beside a fixed fifth made no sense. They are BareButtons now, in a role=group labelled "Value format", with aria-pressed carrying the selection. Keeping mousedown was the reason they were spans. The editor is contenteditable and pressing down on a button blurs it, losing the caret. preventDefault on mousedown stops the blur without stopping the click, so onClick can do the work and the keyboard path comes for free. The styles keyed off the `span` selector, so they move to `.bare-btn`. BareButton resets at zero specificity, so these rules win without having to out-specify it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three label treatments had grown up around one component: FieldLabel at two call sites, a raw <label> at two, a bespoke ValueRowLabel div at three, and nothing at all in the SAML modal. None of them passed htmlFor, and ValueEditor accepted no id, so no version named the editor. ValueEditor now takes `label` and `labelTooltip` and renders the FieldLabel itself, generating both ids so a caller cannot get the association wrong. The editor carries aria-labelledby, and the contenteditable gets role=textbox with aria-multiline, without which a screen reader announces it as plain text and aria-labelledby has nothing to name. That also puts the label and the format buttons in one flex row. They used to be separate layers, the button row absolutely positioned over whatever label the caller happened to render, so nothing stopped a long label running underneath it. The multivariate control value is the widest case (label, weight chip and tooltip); at 380px, narrower than the drawer gets, it now leaves a 56px gap instead of relying on there being room. The copy button moves into a __field wrapper so it stays positioned against the editor rather than the whole component, which now includes the header. Two onChange handlers were annotated as taking a ChangeEvent when ValueEditor has only ever passed a string; safeParseEventValue returns non-events unchanged, which is why nobody noticed. Typed properly and the wrapper dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ValueEditor was the wrong component here. onlyOneLang turned off the format buttons, the copy control and, because validation renders beside the active format label, the XML validation too. What was left was syntax highlighting on a blob users paste and rarely read, bought at the cost of a contenteditable standing in for a form control. InputGroup with textarea gives what the field needs: a real <textarea> with an id, a FieldLabel wired to it with htmlFor, aria-invalid and aria-describedby, FieldError, and a placeholder that is honoured. The call site has been passing one all along that ValueEditor dropped. The download action moves to the footer beside the sibling "Download Service Provider Metadata" button, rather than hanging off the field's label. It also sends the saved config rather than what the field shows, so in the header it read as "download this field" and would have handed you something else once you typed. data-test and name were 'featureValue', copy-pasted from the feature form, so an unrelated field shared a test id. onlyOneLang existed for this one caller, so it goes with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The feature drawer rendered "Control Value" with a weight chip; segment overrides rendered the same idea as "Segment Control Value - 50%". Same concept, two treatments, and only one of them a chip. ControlWeightChip renders it once. It uses the canonical Chip rather than the legacy .chip class the feature drawer had inline: the legacy one carries a hand-written .dark block, and new code should not add a 36th usage of what #6606 is trying to retire. The chip sits after the tooltip icon, so the label reads "Control Value (i) 100%". It is a sibling of FieldLabel inside a flex row rather than label content, which leaves FieldLabel alone and keeps the weight out of the editor's accessible name: that name is now a stable "Control Value" instead of shifting with the split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ides web/styles/3rdParty/_hljs.scss is for overriding highlight.js and Pylon's chat widget. About 130 of its 387 lines were first-party ValueEditor styles: the component block, its header, field and copy button, the format row, and code.txt. They now live in ValueEditor.scss beside the component, which drops _hljs.scss to 256 lines of what it says it is. The selectors are unchanged, so specificity and cascade behave as before. Verified in the browser: copy still 18px/12px inside the field and 11px in code-medium, padding still 16px 44px 16px 20px, code-medium still holds 44px, the format buttons still 12px, and code.txt keeps its 14px/500 OpenSans on a white surface. It is the only component stylesheet that imports _variables. The six input sizing values it needs have no token equivalent, and inlining them would lose the link to $input-height that code-medium exists to match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ount The language check ran in a mount-only effect behind a disabled exhaustive-deps rule. Values load after mount, and when one did the effect had already returned at `if (!value)`, so a JSON value rendered as plaintext for the life of the editor. It now waits for a value and runs once, tracked by a ref that a manual pick also sets, so detection never overrides a choice. The caller's `language` seeds the state directly and counts as already settled. No lint suppression needed. Alongside, from the review pass: Scope `code.txt`. It was a global selector, so any <code class="txt"> in the app picked up a white surface, a border and 14px OpenSans. Finish moving the styles out of the globals: a `.value-editor` block was still in styles.scss. Its disabled-state cursor and opacity move here; the rule hiding the format row is dead, since the row is no longer rendered while disabled. Drop `editorId`. Nothing referenced it: the label association goes through aria-labelledby, because a <code> is not labelable and htmlFor would do nothing. Highlight's id passthrough goes with it. Import Row and Tooltip rather than reading them off the window, which is why Storybook needed stubs for them. Adds a story for a value that arrives after mount, covering the bug above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The parse check lived inside LanguageValidation, which renders inside LanguageSelector, so validity only existed when the format row did. That is why the SAML field could ask for language='xml' and get no XML validation: hiding the row hid the check with it. ValueEditor computes it now and passes it down, so LanguageValidation is presentational and can be rendered anywhere. Callers can read it too, via onValidityChange. saveFeatureWithValidation still reads 'language-validation-error' off the DOM, so that id stays. Moving it onto the callback needs a decision this change should not make on its own: the DOM query is global, so today a parse error in a segment override also blocks saving the feature value, and a callback has to say which editors gate which save. Also documents why onChange hands back a string rather than a FlagsmithValue: this edits text, and deciding "123" is a number is domain logic that belongs to the caller. Two SegmentOverrides handlers were still annotated as taking events and wrapping in safeParseEventValue, which has been a no-op since ValueEditor started passing strings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
talissoncosta
force-pushed
the
refactor/value-editor-typescript
branch
from
September 4, 2026 20:31
0d08e41 to
fea4a29
Compare
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.
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
#8446 has merged, so this is rebased onto
mainand stands on its own.No behaviour change intended.
ValueEditorwas a 254-line untyped class with no props contract, which is howplaceholderandreadOnlycame to be passed at three call sites and silently ignored.TypeScript. Renaming to
.tsxproduced 43 strict errors. All but two were missing prop/state interfaces and implicitany. The two that were not:renderValidation()was being called with an argument it does not accept<Tooltip position='top'>passes a propTooltipPropsdoes not declarevalueis nowFlagsmithValuerather thanstring, which is what the MV and segment call sites actually pass. That surfaced immediately once the interface existed.Split by concern. Each piece is small, and the parsing is unit-testable on its own for the first time:
Imports are unchanged at all eight call sites: the folder resolves where the file used to.
Dropped ionicons from this component. We already own
warningandcheckmark-circle, so it is a straight swap.The format labels are real buttons now. They were
<span onMouseDown>: not focusable, no role, no keyboard path. Same defect the copy control had in #8446, and leaving four of them beside a fixed fifth made no sense. They areBareButtons in arole="group"labelled "Value format", witharia-pressedcarrying the selection.mousedownwas the reason they were spans: the editor iscontenteditable, and pressing down on a button blurs it and loses the caret.preventDefaulton mousedown stops the blur without stopping the click, soonClickdoes the work and the keyboard path comes for free. Verified in the browser: mousedown is default-prevented, focus stays in the editor, and the language still changes.Stopped using
polyfill-react-nativehere. ItsClipboardwraps the deprecateddocument.execCommand. Copying now goes throughnavigator.clipboard, whichlib.domalready types.To get there without importing the
Utilsobject (whose Storybook stub explicitly asks new components not to),copyToClipboardis extracted intocommon/utils/copyToClipboard.Utilskeeps the key and delegates, so its nineteen existing callers are untouched.How did you test this code?
ValueEditoris clean undertsc --strict(the only remaining error in its story isCannot find module 'storybook', which every story in the repo reports).Drove the Storybook stories from #8446 in the browser and asserted on the rendered DOM in both themes: format label order, active language, validation icon present with the right tone class, copy still a
<button>with itsaria-label, and noion-iconelement anywhere.That caught two regressions I had introduced and have fixed: the format labels came out alphabetically ordered, and the validation icon vanished.
ValueEditor owns its label now. Three treatments had grown up around one component:
FieldLabelat two call sites, a raw<label>at two, a bespokeValueRowLabeldiv at three, nothing in the SAML modal. None passedhtmlFor, andValueEditoraccepted noid, so none of them actually named the editor.It now takes
labelandlabelTooltipand renders theFieldLabelitself, generating both ids so a caller cannot get the association wrong. The editor carriesaria-labelledby, and the contenteditable getsrole="textbox"witharia-multiline— without a role, a screen reader announces it as plain text andaria-labelledbyhas nothing to name.That also settles the layout problem I raised earlier. Label and format buttons are now one flex row instead of two layers, so a long label can no longer run under the buttons. The multivariate control value is the widest case (label + weight chip + tooltip); measured at 380px, narrower than the drawer gets, it leaves a 56px gap rather than relying on there being room. Two stories cover it:
BadgeLabelandBadgeLabelNarrow.Tests
13 new unit tests, all passing (
npm run test:unit).validate.ts— every language's valid and invalid paths. Writing them caught a wrong assumption of mine:a:\n- b\n - cis valid YAML, so the failure case now uses an unclosed quote and a bad indent.copyToClipboard.ts— writes the value, default and caller-supplied messages, and that a rejected write toasts and rethrows.Both are plain
.tswith no React, which is the only thing this project can unit test today: jest runstestEnvironment: 'node',testMatchpicks up.tsbut not.tsx, and there is no React Testing Library.What is still not covered, honestly:
LanguageSelector,LanguageValidation,CopyValueButtonandValueEditoritself have no behavioural tests. The Storybook stories are visual coverage via Chromatic, not tests.xmlbranch, left asit.todo. It needsDOMParser, which the node environment does not provide. Addingjest-environment-jsdomis a wider decision than this file.data-testselectors on the<textarea>that theE2E ? textarea : Highlightfork swaps in. They never exerciseHighlight, the contenteditable that production actually renders. That is worth knowing before anyone reads the E2E suite as coverage of this component.Reviewing this
Three things worth a look:
IcondropsclassNamefor most icons. It only spreads...restin 12 of ~100 cases, andwarninghardcodes#FF9F43rather thancurrentColor. I worked around it here with a wrapper span plus explicitfill='currentColor', rather than changingIconand taking the blast radius. It is the same defect the design system audit flagged, and it deserves its own issue.The validation icon is not actually red or green.
_hljs.scsshas.select-language .active .language-icon { color: var(--color-text-default) }, which out-specifies.text-danger/.text-success. Since validation only ever renders on the active label, that rule always wins. This is pre-existing, not something this PR introduced, and I left it alone so the diff stays a refactor. But an error icon rendering in body colour looks wrong and is a one-line fix once someone confirms the intent.copyToClipboardextraction. Moving one function out ofUtilsand re-exporting it is a pattern, not a one-off, and it is the direction the Storybook stub asks for. Say if you would rather it stayed insideUtilsand I reverted the extraction.data-testis still there, deliberately. It is used in 11 places across 5 E2E files. It cannot move to a11y selectors yet: the editable surface is acontenteditable<code>with noroleand no accessible name, and neither does the<textarea>that E2E swaps in. That is a real WCAG 4.1.2 gap, and it is also why theE2E ? textarea : Highlightfork exists at all, which means E2E never exercises the real editor. Giving the editor a role and a name would fix the a11y defect, let the tests usegetByRole('textbox', { name: 'Value' }), and potentially delete that fork. Worth its own PR.Still not done
aria-pressedtoggle buttons, not aradiogroup. A radiogroup is more precise for a mutually exclusive set, but wants roving tabindex and arrow-key navigation. Tab-to-each plus Enter/Space is accessible today; say if you would rather have the full radiogroup.