Skip to content

Commit 4e0edeb

Browse files
talissoncostaclaude
andcommitted
refactor(value-editor): own validity instead of the format row
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>
1 parent c31e024 commit 4e0edeb

4 files changed

Lines changed: 48 additions & 28 deletions

File tree

frontend/web/components/SegmentOverrides.js

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -217,11 +217,7 @@ const SegmentOverrideInner = class Override extends React.Component {
217217
onCompare={onCompare}
218218
onCopyValue={() => {
219219
this.setState({ changed: true })
220-
setValue(
221-
Utils.getTypedValue(
222-
Utils.safeParseEventValue(controlValue),
223-
),
224-
)
220+
setValue(Utils.getTypedValue(controlValue))
225221
}}
226222
canCopyValue={
227223
permission &&
@@ -286,11 +282,9 @@ const SegmentOverrideInner = class Override extends React.Component {
286282
onChange={
287283
readOnly
288284
? null
289-
: (e) => {
285+
: (newValue) => {
290286
this.setState({ changed: true })
291-
setValue(
292-
Utils.getTypedValue(Utils.safeParseEventValue(e)),
293-
)
287+
setValue(Utils.getTypedValue(newValue))
294288
}
295289
}
296290
placeholder="Value e.g. 'big' "
@@ -307,11 +301,9 @@ const SegmentOverrideInner = class Override extends React.Component {
307301
onChange={
308302
readOnly
309303
? null
310-
: (e) => {
304+
: (newValue) => {
311305
this.setState({ changed: true })
312-
setValue(
313-
Utils.getTypedValue(Utils.safeParseEventValue(e)),
314-
)
306+
setValue(Utils.getTypedValue(newValue))
315307
}
316308
}
317309
/>

frontend/web/components/ValueEditor/ValueEditor.tsx

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,12 @@
1-
import React, { FC, ReactNode, useEffect, useId, useRef, useState } from 'react'
1+
import React, {
2+
FC,
3+
ReactNode,
4+
useEffect,
5+
useId,
6+
useMemo,
7+
useRef,
8+
useState,
9+
} from 'react'
210
import cx from 'classnames'
311

412
import FieldLabel from 'components/base/forms/FieldLabel'
@@ -8,6 +16,7 @@ import { FlagsmithValue } from 'common/types/responses'
816
import CopyValueButton from './components/CopyValueButton'
917
import LanguageSelector from './components/LanguageSelector'
1018
import { ValueEditorLanguage } from './types'
19+
import { validateValue } from './validate'
1120

1221
import './ValueEditor.scss'
1322

@@ -25,7 +34,12 @@ export interface ValueEditorProps {
2534
labelTooltip?: string
2635
language?: ValueEditorLanguage
2736
onBlur?: () => void
37+
// The edited text. Deliberately a string, not FlagsmithValue: this edits
38+
// text, and deciding that "123" is a number is Flagsmith's domain logic.
39+
// Callers interpret it (Utils.getTypedValue, Utils.valueToFeatureState).
2840
onChange?: (value: string) => void
41+
// Fires when the value stops or starts parsing under the active format.
42+
onValidityChange?: (error: string | false) => void
2943
value?: FlagsmithValue
3044
}
3145

@@ -38,6 +52,7 @@ const ValueEditor: FC<ValueEditorProps> = ({
3852
language: languageProp,
3953
onBlur,
4054
onChange,
55+
onValidityChange,
4156
value,
4257
}) => {
4358
const [language, setLanguage] = useState<ValueEditorLanguage>(
@@ -62,6 +77,16 @@ const ValueEditor: FC<ValueEditorProps> = ({
6277
} catch (e) {}
6378
}, [text])
6479

80+
// Validity lives here rather than in the format row, so it can be reported
81+
// to callers and rendered anywhere. Keeping it in the row is what left the
82+
// SAML field with language='xml' and no XML validation at all.
83+
const error = useMemo(() => validateValue(language, text), [language, text])
84+
85+
useEffect(() => {
86+
onValidityChange?.(error)
87+
// eslint-disable-next-line react-hooks/exhaustive-deps
88+
}, [error])
89+
6590
const pickLanguage = (next: ValueEditorLanguage) => {
6691
formatSettled.current = true
6792
setLanguage(next)
@@ -90,9 +115,9 @@ const ValueEditor: FC<ValueEditorProps> = ({
90115
)}
91116
{showControls && (
92117
<LanguageSelector
118+
error={error}
93119
language={language}
94120
onChange={pickLanguage}
95-
value={text}
96121
/>
97122
)}
98123
</div>

frontend/web/components/ValueEditor/components/LanguageSelector.tsx

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,16 @@ import LanguageValidation from './LanguageValidation'
1414
interface LanguageSelectorProps {
1515
language: ValueEditorLanguage
1616
onChange: (language: ValueEditorLanguage) => void
17-
value: string
17+
// The active language's parse error, or false. Passed through rather than
18+
// computed here so the row does not own validity.
19+
error: string | false
1820
}
1921

2022
/** The .txt/.json/.xml/.toml/.yaml row above the editor. */
2123
const LanguageSelector: FC<LanguageSelectorProps> = ({
24+
error,
2225
language,
2326
onChange,
24-
value,
2527
}) => (
2628
<Row className='select-language gap-1' role='group' aria-label='Value format'>
2729
{LANGUAGES.map((option) => (
@@ -37,7 +39,7 @@ const LanguageSelector: FC<LanguageSelectorProps> = ({
3739
>
3840
{LANGUAGE_LABELS[option]}{' '}
3941
{option !== 'txt' && language === option && (
40-
<LanguageValidation language={option} value={value} />
42+
<LanguageValidation language={option} error={error} />
4143
)}
4244
</BareButton>
4345
))}

frontend/web/components/ValueEditor/components/LanguageValidation.tsx

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import React, { FC, useMemo } from 'react'
1+
import React, { FC, ReactNode } from 'react'
22

33
import Icon from 'components/icons/Icon'
44
import Tooltip from 'components/Tooltip'
@@ -7,33 +7,32 @@ import {
77
LANGUAGE_LABELS,
88
ValueEditorLanguage,
99
} from 'components/ValueEditor/types'
10-
import { validateValue } from 'components/ValueEditor/validate'
1110

1211
interface LanguageValidationProps {
1312
language: ValueEditorLanguage
14-
value: string
13+
// The parse error for the current value, or false when it is valid. Computed
14+
// by ValueEditor, so this stays presentational and can be rendered anywhere,
15+
// not only inside the format row.
16+
error: string | false
1517
}
1618

1719
// Icon drops className for most icons and only a few spread their rest props,
1820
// so the colour class goes on a wrapper and the icon inherits via currentColor.
1921
const Wrapper: FC<{
2022
tone: 'success' | 'danger'
21-
children: React.ReactNode
23+
children: ReactNode
2224
id?: string
2325
}> = ({ children, id, tone }) => (
2426
<span id={id} className={`language-icon text-${tone}`}>
2527
{children}
2628
</span>
2729
)
2830

29-
/** Tick or warning beside the active format label. */
31+
/** Tick or warning for the current value under the active format. */
3032
const LanguageValidation: FC<LanguageValidationProps> = ({
33+
error,
3134
language,
32-
value,
3335
}) => {
34-
const error = useMemo(() => validateValue(language, value), [language, value])
35-
const name = LANGUAGE_LABELS[language].replace('.', '')
36-
3736
if (!error) {
3837
return (
3938
<Wrapper tone='success'>
@@ -42,11 +41,13 @@ const LanguageValidation: FC<LanguageValidationProps> = ({
4241
)
4342
}
4443

44+
const name = LANGUAGE_LABELS[language].replace('.', '')
4545
return (
4646
<Tooltip
4747
title={
4848
// saveFeatureWithValidation reads this id off the DOM to decide
49-
// whether to warn before saving, so it has to stay.
49+
// whether to warn before saving. ValueEditor now reports validity
50+
// through onValidityChange; the id stays until that caller moves over.
5051
<Wrapper tone='danger' id='language-validation-error'>
5152
<Icon name='warning' width={14} fill='currentColor' />
5253
</Wrapper>

0 commit comments

Comments
 (0)