fix(review): tighten the contract and the format row's markup

From a review pass over the split:

- `label` is required, since the accessible name is the point. That
  caught the story wrapper typed as `Record<string, any>`; it takes the
  component's own props now. ReactNode admits null, so the runtime guard
  still does the work of not rendering a nameless editor.
- `language` is derived from the prop, the user's pick, then detection.
  It was read into useState on mount, so a caller changing the prop kept
  the old format for highlighting and validation. No caller does that
  today.
- Validity is only computed alongside the format row, so a caller that
  pins the format no longer runs a DOMParser parse per keystroke.
- LanguageValidation renders beside the format button rather than inside
  it: Tooltip mounts a div, which a button cannot contain, and it joined
  the button's accessible name while shown.
- The SAML download control is a BareButton with an aria-label, wrapped
  by its tooltip rather than wrapping it. It was a clickable div with no
  keyboard path and no name.
- An invalid-JSON story is back, so the danger tone has visual coverage
  again. The format is clicked rather than pinned, since a pinned editor
  has no row to render the warning against.
- The helper comment claimed the multivariate label reads
  "Control Value <weight>%". It does not: the weight chip is a labelAfter
  sibling, outside the label. Named the `string | Locator` union too.

Verified in Storybook: a value arriving after mount still detects as
json, clicking .xml overrides it and raises the warning, a pinned format
still hides the row, and the warning renders red beside the active label.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: talissoncosta

frontend/documentation/components/ValueEditor.stories.tsx

@@ -2,7 +2,7 @@ import React, { useEffect, useState } from 'react'
 import type { Meta, StoryObj } from 'storybook'
 
 import Constants from 'common/constants'
-import ValueEditor from 'components/ValueEditor'
+import ValueEditor, { ValueEditorProps } from 'components/ValueEditor'
 import ControlWeightChip from 'components/mv/ControlWeightChip'
 
 const meta: Meta = {
@@ -13,11 +13,17 @@ export default meta
 
 type Story = StoryObj
 
+// The real props, so a story cannot drift from the component's contract.
+type InteractiveProps = Omit<ValueEditorProps, 'value' | 'onChange'> & {
+  initialValue?: string
+  width?: number
+}
+
 const Interactive = ({
   initialValue = '',
   width = 640,
   ...props
-}: Record<string, any>) => {
+}: InteractiveProps) => {
   const [value, setValue] = useState(initialValue)
   return (
     <div style={{ maxWidth: width, padding: 16 }}>
@@ -75,6 +81,20 @@ export const ValueArrivesAfterMount: Story = {
   render: () => <LateLoading />,
 }
 
+// The danger tone, which no other story shows. The format has to be chosen
+// rather than pinned: a pinned editor has no format row to render the warning
+// against.
+export const InvalidJson: Story = {
+  play: async ({ canvasElement }: { canvasElement: HTMLElement }) => {
+    const buttons = canvasElement.querySelectorAll('.select-language button')
+    const json = Array.from(buttons).find(
+      (button) => button.textContent?.trim() === '.json',
+    )
+    ;(json as HTMLButtonElement | undefined)?.click()
+  },
+  render: () => <Interactive label='Value' initialValue='{ "colour": ' />,
+}
+
 export const CodeMedium: Story = {
   render: () => (
     <Interactive

frontend/e2e/helpers/e2e-helpers.playwright.ts

@@ -1,4 +1,7 @@
 import { Locator, Page, expect } from '@playwright/test';
+
+// A CSS/data-test string, or a Locator built from role and accessible name.
+type SelectorOrLocator = string | Locator;
 import { LONG_TIMEOUT, SHORT_TIMEOUT, byId, log, logUsingLastSection, getFlagsmith } from './utils.playwright';
 
 // Re-export for backwards compatibility
@@ -19,8 +22,9 @@ export class E2EHelpers {
   constructor(private page: Page) {}
 
   // The value editors are selected by role and accessible name rather than a
-  // data-test. The feature value label switches to "Control Value <weight>%"
-  // once the feature has variations, hence the alternation.
+  // data-test. The label reads "Control Value" once the feature has variations,
+  // hence the alternation; the weight chip is a labelAfter sibling, so it stays
+  // out of the accessible name.
   featureValueField(): Locator {
     return this.page
       .locator('#create-feature-modal')
@@ -44,7 +48,7 @@ export class E2EHelpers {
     return await this.page.locator(byId(selector)).count() > 0;
   }
 
-  async setText(selector: string | Locator, text: string) {
+  async setText(selector: SelectorOrLocator, text: string) {
     logUsingLastSection(`Set text ${selector} : ${text}`);
     const element = typeof selector === 'string' ? this.page.locator(selector).first() : selector;
     await element.waitFor({ state: 'visible', timeout: LONG_TIMEOUT });
@@ -54,7 +58,7 @@ export class E2EHelpers {
     }
   }
 
-  async waitForElementVisible(selector: string | Locator, timeout: number = LONG_TIMEOUT) {
+  async waitForElementVisible(selector: SelectorOrLocator, timeout: number = LONG_TIMEOUT) {
     logUsingLastSection(`Waiting element visible ${selector}`);
     const element = typeof selector === 'string' ? this.page.locator(selector).first() : selector;
     await element.waitFor({

frontend/e2e/tests/change-request-test.pw.ts

@@ -15,8 +15,7 @@ test.describe('Change Request Tests', () => {
       featureValueField,
       assertChangeRequestCount,
       approveChangeRequest,
-      assertInputValue,
-      closeModal,
+        closeModal,
       createChangeRequest,
       createEnvironment,
       createRemoteConfig,

frontend/web/components/ValueEditor/ValueEditor.tsx

@@ -23,9 +23,9 @@ import './ValueEditor.scss'
 export interface ValueEditorProps {
   className?: string
   disabled?: boolean
-  // Rendered as a FieldLabel wired to the editor, so callers cannot get the
-  // association wrong.
-  label?: ReactNode
+  // Required: it is what names the editor. Rendered as a FieldLabel wired to
+  // it, so callers cannot get the association wrong.
+  label: ReactNode
   // Sits beside the label, outside it, so it stays out of the editor's
   // accessible name.
   labelAfter?: ReactNode
@@ -49,37 +49,41 @@ const ValueEditor: FC<ValueEditorProps> = ({
   onChange,
   value,
 }) => {
-  const [language, setLanguage] = useState<ValueEditorLanguage>(
-    languageProp ?? 'txt',
-  )
+  const [picked, setPicked] = useState<ValueEditorLanguage>()
+  const [detected, setDetected] = useState<ValueEditorLanguage>()
+  // Derived, not state: a caller that changes `language` has to win on the
+  // next render, and useState would keep whatever it read on mount.
+  const language = languageProp ?? picked ?? detected ?? 'txt'
   const labelId = useId()
   const text = value === undefined || value === null ? '' : `${value}`
 
   // Detection waits for a value rather than running on mount: values load
   // after mount, and a mount-only check left JSON rendering as plaintext.
-  const formatSettled = useRef(!!languageProp)
+  const detectionDone = useRef(false)
 
   useEffect(() => {
-    if (formatSettled.current || !text) return
-    formatSettled.current = true
+    if (languageProp || picked || detectionDone.current || !text) return
+    detectionDone.current = true
     try {
       if (typeof JSON.parse(text) === 'object') {
-        setLanguage('json')
+        setDetected('json')
       }
     } catch (e) {}
-  }, [text])
-
-  const error = useMemo(() => validateValue(language, text), [language, text])
+  }, [text, languageProp, picked])
 
-  const pickLanguage = (next: ValueEditorLanguage) => {
-    formatSettled.current = true
-    setLanguage(next)
-  }
+  const pickLanguage = (next: ValueEditorLanguage) => setPicked(next)
 
   // A caller that pins the format has nothing to switch, so the row goes, and
   // copy goes with it as it always has.
   const showControls = !disabled && !languageProp
 
+  // Only rendered alongside the format row, so a pinned caller does not need a
+  // DOMParser run on every keystroke.
+  const error = useMemo(
+    () => (showControls ? validateValue(language, text) : false),
+    [language, text, showControls],
+  )
+
   return (
     <div
       className={cx(

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

@@ -1,4 +1,4 @@
-import React, { FC, MouseEvent } from 'react'
+import React, { FC, Fragment, MouseEvent } from 'react'
 import cx from 'classnames'
 
 import BareButton from 'components/base/forms/BareButton'
@@ -30,22 +30,26 @@ const LanguageSelector: FC<LanguageSelectorProps> = ({
     aria-label='Value format'
   >
     {LANGUAGES.map((option) => (
-      <BareButton
-        key={option}
-        // The editor is contenteditable: pressing down on a button would blur
-        // it. preventDefault keeps the caret; the click still fires.
-        onMouseDown={(e: MouseEvent) => e.preventDefault()}
-        onClick={() => onChange(option)}
-        aria-pressed={language === option}
-        className={cx(option, 'd-flex align-items-center text-secondary', {
-          active: language === option,
-        })}
-      >
-        {LANGUAGE_LABELS[option]}{' '}
+      <Fragment key={option}>
+        <BareButton
+          // The editor is contenteditable: pressing down on a button would
+          // blur it. preventDefault keeps the caret; the click still fires.
+          onMouseDown={(e: MouseEvent) => e.preventDefault()}
+          onClick={() => onChange(option)}
+          aria-pressed={language === option}
+          className={cx(option, 'd-flex align-items-center text-secondary', {
+            active: language === option,
+          })}
+        >
+          {LANGUAGE_LABELS[option]}
+        </BareButton>
+        {/* Beside the active button, not inside it: the tooltip mounts a div,
+            which a button cannot contain, and it would otherwise join the
+            button's accessible name while shown. */}
         {option !== 'txt' && language === option && (
           <LanguageValidation language={option} error={error} />
         )}
-      </BareButton>
+      </Fragment>
     ))}
   </Row>
 )

frontend/web/components/pages/organisation-settings/tabs/sso/saml/modals/CreateSAML.tsx

@@ -1,5 +1,6 @@
 import React, { FC, useEffect, useState } from 'react'
 import FieldLabel from 'components/base/forms/FieldLabel'
+import BareButton from 'components/base/forms/BareButton'
 import InputGroup from 'components/base/forms/InputGroup'
 import Utils from 'common/utils/utils'
 import Switch from 'components/Switch'
@@ -167,20 +168,27 @@ const CreateSAML: FC<CreateSAML> = ({ organisationId, samlName }) => {
               label='IdP metadata XML'
               labelAfter={
                 data?.idp_metadata_xml && (
-                  <div className='clickable' onClick={downloadIDPMetadata}>
-                    <Tooltip
-                      title={
+                  // A button, not a clickable div: this was unreachable by
+                  // keyboard and had no accessible name. The tooltip wraps it
+                  // rather than nesting inside, since it renders a div.
+                  <Tooltip
+                    title={
+                      <BareButton
+                        aria-label='Download IdP metadata'
+                        className='d-inline-flex align-items-center'
+                        onClick={downloadIDPMetadata}
+                      >
                         <IonIcon
                           className='icon-action'
                           icon={cloudDownloadOutline}
                           style={{ fontSize: '18px' }}
                         />
-                      }
-                      place='right'
-                    >
-                      Download IDP Metadata
-                    </Tooltip>
-                  </div>
+                      </BareButton>
+                    }
+                    place='right'
+                  >
+                    Download IdP metadata
+                  </Tooltip>
                 )
               }
               className='full-width'