feat(apollo-wind): string-list field, tooltip metadata, and forms repairs - #1107
Merged
Merged
Conversation
apetraru-uipath
added
the
dev-packages
Contributor
|
Apollo Coded App preview deployments are ready.
|
Contributor
Dependency License Review
License distribution
Excluded packages
|
Contributor
📦 Dev Packages🧹 Dev packages cleaned up after PR close. Last updated: 2026-09-14 11:44:25 PT |
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces the first shared “Guardrails” component family in apollo-wind by adding GuardrailValidatorForm (a fully controlled, host-validated parameter editor) along with supporting types, i18n catalog loading, and save-time utilities. This also begins exposing components/custom as a public API surface for the package.
Changes:
- Added
GuardrailValidatorFormwith editor support for 7 parameter types plus an override slot (renderParameter) for product-specific editors. - Added guardrails utilities (
seedGuardrailParameters,syncMapEnumParameters,dropEmptyOptionalParameters,getRequiredEmptyParameterIds) and a lightweight locale-catalog loader with 14 locale modules. - Updated barrels/exports (
src/index.ts,src/components/*, andpackage.jsonexports) and adjusted.gitignoreto include handwritten locale TS sources.
File summaries
| File | Description |
|---|---|
| packages/apollo-wind/src/index.ts | Exposes the guardrails component family + utilities/types from the package root entrypoint. |
| packages/apollo-wind/src/components/index.ts | Adds custom/ shelf to the components barrel. |
| packages/apollo-wind/src/components/custom/index.ts | Publicly exports only the reviewed guardrails family from custom/. |
| packages/apollo-wind/src/components/custom/guardrails/index.ts | Guardrails family barrel (component, types, i18n, utilities). |
| packages/apollo-wind/src/components/custom/guardrails/types.ts | Defines the parameter/definition unions and the GuardrailValidatorFormProps contract. |
| packages/apollo-wind/src/components/custom/guardrails/utils.ts | Adds seeding/cleanup/sync helpers and the required-empty predicate for hosts. |
| packages/apollo-wind/src/components/custom/guardrails/utils.test.ts | Unit coverage for guardrails utilities (seeding, syncing, pruning, required-empty predicate). |
| packages/apollo-wind/src/components/custom/guardrails/i18n.ts | Defines chrome-string labels, default English labels, interpolation and merge logic. |
| packages/apollo-wind/src/components/custom/guardrails/load-messages.ts | Adds supported-locale list, locale normalization, and lazy catalog loading. |
| packages/apollo-wind/src/components/custom/guardrails/load-messages.test.ts | Tests locale resolution, catalog loading/fallback behavior, and label merging. |
| packages/apollo-wind/src/components/custom/guardrails/guardrail-validator-form.tsx | Implements the main controlled form renderer and async label resolution hook. |
| packages/apollo-wind/src/components/custom/guardrails/guardrail-validator-form.test.tsx | Comprehensive RTL + jest-axe coverage across all parameter types, overrides, i18n, a11y. |
| packages/apollo-wind/src/components/custom/guardrails/guardrail-validator-form.stories.tsx | Storybook coverage for interactive scenarios, errors, override slot, and localization. |
| packages/apollo-wind/src/components/custom/guardrails/components/parameter-label.tsx | Shared label + tooltip + inline error rendering primitives for parameter fields. |
| packages/apollo-wind/src/components/custom/guardrails/components/number-parameter-field.tsx | Number editor field implementation. |
| packages/apollo-wind/src/components/custom/guardrails/components/text-parameter-field.tsx | Multiline text editor field implementation. |
| packages/apollo-wind/src/components/custom/guardrails/components/boolean-parameter-field.tsx | Boolean editor field implementation. |
| packages/apollo-wind/src/components/custom/guardrails/components/enum-parameter-field.tsx | Enum select editor (including “synthetic option” behavior for stale stored values). |
| packages/apollo-wind/src/components/custom/guardrails/components/enum-list-parameter-field.tsx | Enum-list chips editor with popover fallback for long option lists. |
| packages/apollo-wind/src/components/custom/guardrails/components/map-enum-parameter-field.tsx | Map-enum editor rendering rows derived from a sibling enum-list parameter. |
| packages/apollo-wind/src/components/custom/guardrails/components/text-list-parameter-field.tsx | Text-list repeated-row editor with add/remove and stable-row key strategy. |
| packages/apollo-wind/src/components/custom/guardrails/README.md | Local package documentation for contract, utilities, localization, and shadow-DOM consumption notes. |
| packages/apollo-wind/src/components/custom/guardrails/locales/en.ts | English chrome-string catalog module. |
| packages/apollo-wind/src/components/custom/guardrails/locales/de.ts | German chrome-string catalog module. |
| packages/apollo-wind/src/components/custom/guardrails/locales/es.ts | Spanish chrome-string catalog module. |
| packages/apollo-wind/src/components/custom/guardrails/locales/es-MX.ts | Spanish (Mexico) chrome-string catalog module. |
| packages/apollo-wind/src/components/custom/guardrails/locales/fr.ts | French chrome-string catalog module. |
| packages/apollo-wind/src/components/custom/guardrails/locales/ja.ts | Japanese chrome-string catalog module. |
| packages/apollo-wind/src/components/custom/guardrails/locales/ko.ts | Korean chrome-string catalog module. |
| packages/apollo-wind/src/components/custom/guardrails/locales/pt.ts | Portuguese chrome-string catalog module. |
| packages/apollo-wind/src/components/custom/guardrails/locales/pt-BR.ts | Portuguese (Brazil) chrome-string catalog module. |
| packages/apollo-wind/src/components/custom/guardrails/locales/ro.ts | Romanian chrome-string catalog module. |
| packages/apollo-wind/src/components/custom/guardrails/locales/ru.ts | Russian chrome-string catalog module (currently empty; relies on per-key English fallback). |
| packages/apollo-wind/src/components/custom/guardrails/locales/tr.ts | Turkish chrome-string catalog module. |
| packages/apollo-wind/src/components/custom/guardrails/locales/zh-CN.ts | Chinese (Simplified) chrome-string catalog module. |
| packages/apollo-wind/src/components/custom/guardrails/locales/zh-TW.ts | Chinese (Traditional) chrome-string catalog module. |
| packages/apollo-wind/package.json | Adds ./components/custom exports to publish the new public custom shelf. |
| .gitignore | Un-ignores the handwritten guardrails locale TS sources under packages/apollo-wind/.../locales/*.ts. |
Review details
- Files reviewed: 37/38 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Storybook visual diffBaseline is the deployed main Storybook, so changes merged to main after this branch was last updated can also appear here. Logs Updated (PT): Sep 14, 2026, 02:39:07 AM |
Contributor
📊 Coverage + size by packagePer-package coverage and bundle size on this PR. New-line coverage = of the source lines this PR adds or changes, the % hit by tests.
"Coverage" is each package's own |
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
packages/apollo-wind/package.json:52
- The
./components/custom/*export will expose every top-level module undersrc/components/custom/(e.g.canvas-studio.tsx,chat-*,panel-*), even thoughsrc/components/custom/index.tsexplicitly says only reviewed families should be public. It also likely won’t resolvecomponents/custom/guardrailscorrectly because the built output will bedist/components/custom/guardrails/index.*, notdist/components/custom/guardrails.*.
Consider removing the wildcard export and adding a single explicit ./components/custom/guardrails export instead (root @uipath/apollo-wind already re-exports the guardrails API).
"./components/custom/*": {
"import": "./dist/components/custom/*.js",
"require": "./dist/components/custom/*.cjs",
"types": "./dist/components/custom/*.d.ts"
},
packages/apollo-wind/src/components/custom/guardrails/components/text-list-parameter-field.tsx:39
- Calling
setStateduring render (if (prevLength !== items.length) { ... }) is an anti-pattern and can trigger React warnings, extra renders, or infinite loops (especially under StrictMode’s double-invocation). SyncrowIdsin an effect keyed onitems.lengthinstead of mutating state while rendering.
// Stable per-row ids: without them, removing index N would reuse the DOM node of a
// surviving sibling and swap a focused textarea's content with someone else's. Resync
// during render (React's official pattern) when the `items` array length changes
// out-of-band; the local add/remove handlers below already mutate ids in lockstep.
const [rowIds, setRowIds] = useState<string[]>(() => items.map(() => crypto.randomUUID()));
- Files reviewed: 37/38 changed files
- Comments generated: 1
- Review effort level: Lite
BenGSchulz
reviewed
Sep 1, 2026
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packages/apollo-wind/src/components/custom/guardrails/components/text-list-parameter-field.tsx:44
- This component updates state during render (
setPrevLength/setRowIdsinside the render body). This can trigger React warnings and can behave poorly under StrictMode/double-invocation; the resync should be done in an effect (e.g.,useEffectkeyed onitems.length) or by restructuring state to avoid derived state.
const [prevLength, setPrevLength] = useState(items.length);
if (prevLength !== items.length) {
setPrevLength(items.length);
setRowIds((prev) =>
prev.length < items.length
- Files reviewed: 56/57 changed files
- Comments generated: 4
- Review effort level: Lite
apetraru-uipath
changed the title
There was a problem hiding this comment.
🔵 Needs a closer look
There are concrete correctness/consumption issues (notably exports subpath mapping and a strict-typing/indexing bug in syncMapEnumParameters), plus a React anti-pattern (state updates during render) that should be addressed before merge.
Review details
Suppressed comments (3)
packages/apollo-wind/src/components/custom/guardrails/utils.ts:72
syncMapEnumParametersbuildscurrentMapasparam.value ? param.value : {}. That widens the type to{}and makescurrentMap[key]unsafe (and can fail TypeScript strict indexing). If you want the runtime guard, keep the type asRecord<string, number>(and also exclude arrays) before indexing.
packages/apollo-wind/package.json:52- The
./components/custom/*export maps to./dist/components/custom/*.js, but this package outputs nestedindex.jsfiles for folder entrypoints (e.g.src/components/custom/guardrails/index.ts→dist/components/custom/guardrails/index.js). As written, imports like@uipath/apollo-wind/components/custom/guardrailsare likely to fail resolution.
"./components/custom/*": {
"import": "./dist/components/custom/*.js",
"require": "./dist/components/custom/*.cjs",
"types": "./dist/components/custom/*.d.ts"
},
packages/apollo-wind/src/components/custom/guardrails/components/text-list-parameter-field.tsx:44
- This component calls
setPrevLength/setRowIdsduring render whenitems.lengthchanges. Updating state during render is not safe in React (can trigger render loops and breaks concurrent rendering assumptions). Please move the resync logic into an effect keyed onitems.length, or use auseRef-backed id list that you mutate synchronously withoutsetState.
const [rowIds, setRowIds] = useState<string[]>(() => items.map(() => crypto.randomUUID()));
const [prevLength, setPrevLength] = useState(items.length);
if (prevLength !== items.length) {
setPrevLength(items.length);
setRowIds((prev) =>
- Files reviewed: 63/64 changed files
- Comments generated: 0 new
- Review effort level: Lite
apetraru-uipath
force-pushed
the
feat/apollo-wind-guardrail-validator-form
branch
from
September 7, 2026 10:09
7833830 to
67fba2d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of correctness issues in the new code and package exports (React state updates during render in text-list-parameter-field, and a likely-broken ./components/custom/* export mapping for directory-based entries).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
packages/apollo-wind/src/components/custom/guardrails/components/text-list-parameter-field.tsx:2
- This file uses
useEffectbelow (or should) to syncrowIds, butuseEffectis not imported; without it, the suggested fix for the row-id syncing will not compile.
import { useCallback, useMemo, useState } from 'react';
packages/apollo-wind/src/components/custom/guardrails/components/text-list-parameter-field.tsx:44
- Calling
setStateduring render (if (prevLength !== items.length) { ... }) is a React anti-pattern and can cause extra render passes or warnings under Strict/Concurrent rendering. Move this resync into an effect keyed onitems.lengthso it only runs after render.
const [prevLength, setPrevLength] = useState(items.length);
if (prevLength !== items.length) {
setPrevLength(items.length);
setRowIds((prev) =>
packages/apollo-wind/src/components/custom/guardrails/utils.ts:70
currentMapis intended to defend against malformed persisted values, but the current check treats arrays as valid objects (sincetypeof [] === 'object'). Ifparam.valueis accidentally an array, indexing into it will yield unexpected results. Consider explicitly excluding arrays when normalizing the persisted map.
packages/apollo-wind/package.json:52- The
./components/custom/*export target points to./dist/components/custom/*.js, but the guardrails family is emitted as a directory (components/custom/guardrails/index.js) under the current rslib config (source entries include nested paths). As written, deep imports like@uipath/apollo-wind/components/custom/guardrailswill resolve todist/components/custom/guardrails.js(which likely doesn’t exist). Update the export mapping to point at*/indexfiles.
"./components/custom/*": {
"import": "./dist/components/custom/*.js",
"require": "./dist/components/custom/*.cjs",
"types": "./dist/components/custom/*.d.ts"
},
- Files reviewed: 63/64 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a React render-path state update bug in StringListField and exposes unintended public APIs via the ./components/custom/* package export wildcard.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packages/apollo-wind/src/components/forms/string-list-field.tsx:51
setPrevLength/setRowIdsare called during render whenitems.lengthchanges. Updating state while rendering will trigger React warnings (and can cause extra renders or loops under StrictMode). Sync therowIdsarray in an effect keyed byitems.lengthinstead of performing state updates in the render path.
- Files reviewed: 71/72 changed files
- Comments generated: 2
- Review effort level: Lite
apetraru-uipath
force-pushed
the
feat/apollo-wind-guardrail-validator-form
branch
from
September 10, 2026 12:50
289fc85 to
3a12a40
Compare
apetraru-uipath
added a commit
that referenced
this pull request
Sep 10, 2026
Moves the guardrails family out of apollo-wind's custom/ prototype shelf into apollo-react per the #1107 review decision: canvas-adjacent (MUI-free, built on wind primitives and the forms/ MetadataForm engine), exported from the canvas components barrel plus a narrow ./canvas/guardrails subpath. Strings move to lingui: useSafeLingui labels-hooks with explicit guardrails.* ids replace the wind-local catalogs and loader; the 60 keys ship translated in the shared canvas catalog for 13 locales (ru falls back to English per key). Localized templates that cross into plain-string APIs are ICU messages formatted with sentinel values, preserving the {{token}} convention. The family's Tailwind classes ride the existing tailwind.canvas.css scan; adds class-variance-authority (dep) and jest-axe (dev, matcher registered in the shared test setup). Also silences Radix's aria-describedby warning on the description-less builder dialog. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings affect the advertised MetadataForm API, validation, serialization, and accessibility behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (14)
packages/apollo-wind/src/components/forms/field-renderer.tsx:520
- The checkbox branch now participates in the new field error rendering, but the
Checkboxitself still receives noaria-invalid. When validation reports an error, assistive technology therefore sees a valid checkbox even thoughFormFieldErroris displayed. Forward the invalid state to this control.
<FormFieldLabel
htmlFor={field.name}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
className="font-normal"
>
{field.label}
</FormFieldLabel>
packages/apollo-wind/src/components/forms/field-renderer.tsx:561
- Radio validation errors are displayed below the group, but
RadioGroupis not markedaria-invalid. The group should expose the error state so assistive technology can announce that the required choice is invalid.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:742
- Slider validation errors are rendered below the control, but the
Sliderroot is not markedaria-invalid. Required/range failures therefore lack the invalid-state signal exposed on the other renderer paths; forward the conditional error state.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/form-schema.ts:324
- The new
string-listmember is part of the publicFieldType, butFormDesigner's "Complete mapping of all supported field types" still omits it fromFIELD_TYPE_METADATA, which is used to populate the field-type selector. Users of the exported designer therefore cannot create this new field type.
| StringListFieldMetadata
packages/apollo-wind/src/components/forms/form-schema.ts:303
valueTypechanges validation behavior for custom fields, butserializeFieldnever writes it (the custom-field branch stops aftercomponentProps). A designer/JSON round trip therefore drops the declared shape and turns the field back intoz.any(), sorequired/array constraints silently disappear. Serialize this property and add a round-trip regression test.
valueType?: 'string' | 'number' | 'boolean' | 'string-array';
packages/apollo-wind/src/components/forms/metadata-form.stories.tsx:1297
- This story copy says the example enforces
pattern: '\\S', but the schema below has novalidation.pattern, and the converter does not apply scalar string constraints tostring-listarray elements. A whitespace-only row therefore passes despite the documented contract. Either implement/configure per-item pattern validation or remove this claim.
* The `string-list` field type (repeated multiline rows with Add/Remove) and `tooltip` field
* metadata, validated the way the schema already expresses it: `minItems: 1` for "at least
* one row" and `pattern: '\\S'` for "not just whitespace" — no host-side re-implementation.
packages/apollo-wind/src/components/forms/metadata-form.test.tsx:1
actis imported but never referenced in this test file. Because the package enablesnoUnusedLocals, this added import causes the TypeScript test build to fail; remove it.
import { act, render, screen, waitFor, within } from '@testing-library/react';
packages/apollo-wind/src/components/forms/metadata-form.tsx:44
MetadataFormPropsstill exposes only the plugin-based API (plugins,onSubmit, etc.); there is novalues,onValuesChange,errors,disableValidation, or synchronouscomponentsprop, andMetadataFormdoes not consume any of them. This contradicts the controlled-host seam described for this PR, so consumers cannot use the advertised API. Either implement the documented props and behavior, or update the PR contract and follow-up consumers to the plugin-only seam.
export interface MetadataFormProps {
schema: FormSchema;
plugins?: FormPlugin[];
onSubmit?: (data: unknown) => void | Promise<void>;
packages/apollo-wind/src/components/forms/metadata-form.tsx:203
- The new watch callback no longer reads
isInitializedRef, but the ref is still declared and updated above. With this package'snoUnusedLocals: true, the unused local makestscfail; remove the dead ref now that initialization is guarded byinitializingRef.
// Suppresses onValuesChange while the `values` sync-in effect writes fields — safe because
// react-hook-form notifies watch subscribers synchronously inside setValue.
const initializingRef = useRef(false);
packages/apollo-wind/src/components/forms/schema-serializer.ts:252
valueTypeis a new part ofCustomFieldMetadata, but this serializer never writes it. A custom field therefore loses its declared shape after a schema round-trip and falls back toz.any(), sorequiredand list validation silently stop applying. SerializevalueTypewith the other custom-field properties.
if ('addItemLabel' in field && field.addItemLabel) result.addItemLabel = field.addItemLabel;
if ('removeItemAriaLabel' in field && field.removeItemAriaLabel) {
result.removeItemAriaLabel = field.removeItemAriaLabel;
packages/apollo-wind/src/components/forms/string-list-field.tsx:118
StringListFieldMetadatainheritsariaLabel, but every row hard-codes its accessible name fromfield.label. Because this field uses a<span>rather than a label element, a consumer-suppliedariaLabelis the only way to override the row name, and it is currently ignored. Usefield.ariaLabel ?? field.labelas the row-name prefix.
aria-label={`${field.label} ${index + 1}`}
packages/apollo-wind/src/components/forms/validation-converter.ts:56
- The new
isEmptyFieldValuehelper trims whitespace only in the conditional-requiredsuperRefine; the ordinaryrequired: truebranch still usesz.string().min(1), so a static required text field accepts' '. That contradicts the helper's "one definition of empty" contract and makes static and conditional required fields behave differently. Apply the same empty-value check in this branch without trimming the submitted value.
if (isStringType(fieldType, customValueType) && !config.minLength) {
schema = (schema as z.ZodString).min(
1,
config.messages?.required || 'This field is required'
);
packages/apollo-wind/src/components/forms/validation-converter.ts:97
ValidationConfig.customis documented with expressions such asvalue.length > otherField, but this refinement evaluates against{ value }only.otherFieldconsequently resolves toundefined, so that documented cross-field expression fails for every non-empty value. Pass the complete form values into object-level validation, or narrow/update the public contract before enabling this enforcement.
return schema.superRefine((value: unknown, ctx: z.RefinementCtx) => {
if (!RulesEngine.evaluateExpression(expression, { value })) {
ctx.addIssue({ code: 'custom', message });
packages/apollo-wind/src/components/forms/validation-converter.ts:37
- Declaring
valueType: 'number'does not make numeric metadata constraints apply:applyNumberConstraintsis still called with onlyfieldType, somin,max,integer,positive, andnegativeare all ignored for custom numeric fields. This contradicts theCustomFieldMetadatacontract that normal constraints apply after declaring the shape; threadcustomValueTypethrough the numeric path and test it.
customValueType?: CustomValueType
): z.ZodTypeAny {
// Get base schema for field type
let schema = getBaseSchemaForType(fieldType, customValueType);
- Files reviewed: 30/30 changed files
- Comments generated: 2
- Review effort level: Lite
apetraru-uipath
added a commit
that referenced
this pull request
Sep 10, 2026
Moves the guardrails family out of apollo-wind's custom/ prototype shelf into apollo-react per the #1107 review decision: canvas-adjacent (MUI-free, built on wind primitives and the forms/ MetadataForm engine), exported from the canvas components barrel plus a narrow ./canvas/guardrails subpath. Strings move to lingui: useSafeLingui labels-hooks with explicit guardrails.* ids replace the wind-local catalogs and loader; the 60 keys ship translated in the shared canvas catalog for 13 locales (ru falls back to English per key). Localized templates that cross into plain-string APIs are ICU messages formatted with sentinel values, preserving the {{token}} convention. The family's Tailwind classes ride the existing tailwind.canvas.css scan; adds class-variance-authority (dep) and jest-axe (dev, matcher registered in the shared test setup). Also silences Radix's aria-describedby warning on the description-less builder dialog. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate issues affect the MetadataForm API, validation, serialization, keyboard behavior, and accessibility.
Review details
Suppressed comments (15)
Previously missed (1) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/forms/validation-converter.ts:97
ValidationConfig.customis documented inform-schema.tsas supporting expressions such asvalue.length > otherField, but this refinement evaluates against{ value }only. Any sibling identifier resolves toundefined, so valid cross-field expressions are rejected; evaluate the expression with the complete form data at object level while preserving the field error path.
packages/apollo-wind/src/components/forms/field-renderer.tsx:517
- Checkbox fields pass
requiredintoFieldByType, but this label does not forward it toFormFieldLabel; required checkboxes therefore lose the required indicator while the other field types show it. Addrequired={required}here.
<FormFieldLabel
htmlFor={field.name}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
className="font-normal"
packages/apollo-wind/src/components/forms/field-renderer.tsx:537
- The switch branch renders
FormFieldErrorbut does not exposearia-invalidon the switch control. A validation failure is consequently not announced as an invalid state, while the newly wired input-like fields do expose it. Pass the error state through toSwitch.
<FormFieldLabel
htmlFor={field.name}
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
packages/apollo-wind/src/components/forms/field-renderer.tsx:510
- The checkbox branch renders
FormFieldErrorbut never setsaria-invalidon the Radix checkbox. Validation failures are therefore not exposed as invalid to assistive technology, unlike the text, select, file, and string-list branches. Forward the error state to the checkbox control.
<Checkbox
checked={formField.value === true}
onCheckedChange={(checked) => formField.onChange(checked === true)}
disabled={disabled}
id={field.name}
packages/apollo-wind/src/components/forms/field-renderer.tsx:561
- The radio branch renders a field error but never marks its
RadioGroupinvalid. Screen readers therefore receive no invalid-state signal for a failed required/validation rule, unlike the other updated composite control. Forwardaria-invalidto the group.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:676
- The new controlled
StringListFieldnever receivesformField.reffromController, so React Hook Form has no focusable element for this field. On submit, an invalid string-list cannot be focused by RHF'sshouldFocusError, unlike the other built-in fields. Forward a ref (for example to the first row textarea) through this component.
<StringListField
field={field}
value={formField.value as string[] | undefined}
onChange={formField.onChange}
onBlur={formField.onBlur}
packages/apollo-wind/src/components/forms/metadata-form.stories.tsx:1297
- This story comment says the example uses
pattern: '\\S'to reject whitespace-only rows, but the actual schema has nopattern, and the array schema does not apply stringpatternconstraints to each item. The story therefore documents validation that the example does not perform. Remove that claim or add a supported per-item validation mechanism.
* String List + Tooltip
*
* The `string-list` field type (repeated multiline rows with Add/Remove) and `tooltip` field
* metadata, validated the way the schema already expresses it: `minItems: 1` for "at least
* one row" and `pattern: '\\S'` for "not just whitespace" — no host-side re-implementation.
packages/apollo-wind/src/components/forms/metadata-form.test.tsx:1
actis imported here but is not referenced anywhere in this test file. With the repository's unused-import checks this makes the test file fail lint/type checking; remove the unused import.
import { act, render, screen, waitFor, within } from '@testing-library/react';
packages/apollo-wind/src/components/forms/metadata-form.tsx:44
- The PR description says the controlled-host seam adds
values,onValuesChange,errors,disableValidation, and a synchronouscomponentsprop, butMetadataFormPropsstill exposes none of those (custom components are only accepted throughplugins). The new forms README documents the opposite by saying there is no controlled-value prop, so consumers following the stated API cannot compile. Please reconcile the shipped API and documentation with the PR description before merge.
export interface MetadataFormProps {
schema: FormSchema;
plugins?: FormPlugin[];
onSubmit?: (data: unknown) => void | Promise<void>;
packages/apollo-wind/src/components/forms/metadata-form.tsx:364
- This prevents Enter for every
<input>exceptbutton, including checkbox, radio, file, range, and other non-text controls. Incontainer="div"mode that suppresses their native keyboard activation, despite the comment promising to guard only single-line inputs. Restrict the guard to text-like input types or explicitly exempt non-text controls.
if (event.key !== 'Enter' || event.defaultPrevented) return;
const target = event.target as HTMLElement;
if (target instanceof HTMLInputElement && target.type !== 'button') {
event.preventDefault();
packages/apollo-wind/src/components/forms/schema-serializer.ts:253
- While serializing the new custom-field metadata,
valueTypeis never copied. A schema round-trip therefore drops the declared custom shape, causingMetadataFormto fall back toz.any()and silently disablerequired/minItemsvalidation after persistence. SerializevalueTypeand cover the round-trip.
if ('addItemLabel' in field && field.addItemLabel) result.addItemLabel = field.addItemLabel;
if ('removeItemAriaLabel' in field && field.removeItemAriaLabel) {
result.removeItemAriaLabel = field.removeItemAriaLabel;
}
packages/apollo-wind/src/components/forms/string-list-field.tsx:13
formatTemplatepromises to leave unknown tokens untouched, buttoken in valuesalso matches inherited keys such astoStringandconstructor. A template like{{toString}}therefore expands to a function's source instead of remaining unchanged. Use an own-property check before reading the value.
token in values ? String(values[token]) : match
packages/apollo-wind/src/components/forms/validation-converter.ts:56
- The new
isEmptyFieldValuetreats whitespace-only strings as empty, but static required string validation still usesz.string().min(1), which accepts' '. Sincemetadata-formuses the helper only for dynamic required checks, static and conditional required fields now disagree. Use the shared predicate for this base required check.
if (isStringType(fieldType, customValueType) && !config.minLength) {
schema = (schema as z.ZodString).min(
1,
config.messages?.required || 'This field is required'
);
packages/apollo-wind/src/components/forms/validation-converter.ts:150
CustomFieldMetadata.valueTypedocuments that normal metadata constraints apply to custom components, but the number constraint path still recognizes only field typesnumberandslider. A custom field withvalueType: 'number'silently skipsmin,max,integer,positive, andnegative, so its declared validation is not enforced. Include the custom value type in the number check and cover it with a validation test.
switch (customValueType) {
case 'string':
return z.string();
case 'number':
return z.number();
case 'boolean':
return z.boolean();
packages/apollo-wind/src/components/forms/validation-converter.ts:64
- When
requiredis true andminItemsis explicitly0, this branch skips the required non-empty check and Zod's.min(0)accepts[]. That lets a required list submit empty; only a positiveminItemsshould replace the required constraint.
if (isArrayType(fieldType, customValueType) && config.minItems == null) {
schema = (schema as z.ZodArray<z.ZodTypeAny>).min(
1,
config.messages?.required || 'This field is required'
);
- Files reviewed: 30/30 changed files
- Comments generated: 0 new
- Review effort level: Lite
BenGSchulz
reviewed
Sep 10, 2026
BenGSchulz
reviewed
Sep 10, 2026
BenGSchulz
left a comment
BenGSchulz
left a comment
Contributor
There was a problem hiding this comment.
Re-review of the follow-up commits, split inline. Two verified bugs in validation-converter.ts and three leftovers from the revert in metadata-form.tsx. The bridge-side findings are on #1138.
The prop surface is exactly what was asked for: container is the only addition. The container='div' handling went further than I suggested and in the right direction, with submit actions becoming plain buttons and Enter swallowed only for single-line inputs without stopPropagation, so inner handlers still run first.
andreizdrali-uipath
added a commit
that referenced
this pull request
Sep 11, 2026
…AL-574] Review of #1139 against the #1107/#1138 threads (plan/review-2026-09-11 §3.2). - Type the fetch mocks as `vi.fn<typeof fetch>`: `vi.fn(async () => ...)` infers a zero-parameter mock, so every `mock.calls[i]?.[1]` assertion was a TS2493/TS2339 under the repo's strict config. CI cannot see it (tests are excluded from `tsc` and biome does not typecheck), so it is checked with a throwaway tsconfig. - `loading` starts `true` when the hook is about to fetch, so a host rendering `loading ? <Spinner/> : <Empty/>` no longer flashes the empty state on first paint. - `refetch` is a no-op while the hook is disabled. It used to issue a real request whose result `parsed` then discarded in favour of `options.definitions`. - JSDoc and README: `options.definitions` is compared by identity (pass a stable reference), a failed request keeps the previous results, and the zod boundary is pinned by a source-level check plus two tests, not by shipped runtime assertions. - Name the map-enum `0..1` step `0.1` default as a product assumption and pin what keeps it safe: after the rebase onto `d658731b`, `min`/`max` are enforced through `validation` in `onChange` mode, but both that path and `getOutOfRangeParameterIds` are number-only, so a synthesized map-enum bound cannot reject a threshold map whose real range is different (harmful content is 0..6). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
andreizdrali-uipath
added a commit
that referenced
this pull request
Sep 11, 2026
Review of #1140 against the #1107/#1138 threads (plan/review-2026-09-11 §4.3). Tokens, the three themes Ben Schulz already pushed back on in #1107: - The two BYO notices use `text-error`, wind's own error-text token, not `text-destructive`: the two resolve differently in several theme blocks of `tailwind.consumer.css`, and `FormFieldError` settled on `text-error`. - `text-[11px]` twice becomes `text-xs`; the family has no arbitrary type sizes. - The remove button keeps its four-scope destructive ghost override and its comment. A wind `ghost-destructive` variant is the better answer and widens this PR. Accessibility, on the activatable row: - The body is `role="button"`, so ARIA treats everything in it as presentational. The BYO notices and the description now carry ids and are named in the body's `aria-describedby`. The provider line, action badge and scopes stay presentational and the README says so, since naming all of them turns one announcement into a paragraph. - A tooltipped body that is not activatable gets `tabIndex={0}`: Radix opens on focus as well as hover, so that is all the tooltip needed to stop being pointer-only. - The notices keep `role="alert"` rather than the family banner's `role="status"`, with the reason in place: Flow announces them on mount today. Claims match code: - `capitalize` applies only to the raw `$actionType` branch, never to host-localized `formatAction` output, which is already cased for its locale. - The sensors move into a `SortableRows` component next to the `DndContext`, so "with reorder off, no drag machinery mounts" is literally true rather than nearly: `useSensor`/`useSensors` are hooks and used to run for every list. - `GuardrailListItemActionsContext.onEdit`/`onRemove` are documented as absent when the host passed no handler *or* the list is disabled. - `GuardrailRowTooltipRenderer` is declared once in `list-types.ts` and exported, instead of the list indexing into the row's non-exported props. README rows and tests cover each behaviour change: 91 tests across the five suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
apetraru-uipath
added a commit
that referenced
this pull request
Sep 11, 2026
…ling InfoTooltip moves out of the guardrails prototype into components/ui with its a11y test; select and textarea get aria-invalid error styling; the root barrel exposes the new forms/ui surface. The guardrails domain family itself moves to apollo-react (canvas) per the #1107 review decision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues affect the advertised host API, validation behavior, tooltip providers, and string-list designer support.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (10)
packages/apollo-wind/src/components/forms/README.md:20
- The PR description says
MetadataFormexposesvalues,onValuesChange,errors,disableValidation, and a directcomponentsregistry, but this newly documented contract explicitly says there is no controlled-value prop andMetadataFormPropsexposes none of those props. Consumers following the advertised controlled-host seam therefore cannot compile or use it; either implement that API or correct the PR description and dependent consumer contract.
There is deliberately no controlled-value prop. A second source of truth for values means
two things can disagree about what the user typed, and every consumer then reimplements the
reconciliation. `context.form` is the full `UseFormReturn`, so anything RHF can do, a plugin
packages/apollo-wind/src/components/forms/field-renderer.tsx:538
- This branch now renders the required indicator for switches, but the converter has no required rule for booleans (
z.boolean()acceptsfalse). A schema withvalidation: { required: true }therefore marks an unchecked switch as required while submission still succeeds. Either enforcetruefor required boolean fields or omit the indicator and align this branch with checkbox semantics.
<FormFieldLabel
htmlFor={field.name}
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
packages/apollo-wind/src/components/forms/form-schema.ts:328
string-listis now part of the publicFieldMetadataunion and has a renderer, butFIELD_TYPE_METADATAinform-designer.tsxstill omits it despite describing itself as the complete mapping of supported field types. The shippedFormDesignertherefore cannot create or edit this new field type. Add the designer support, or explicitly document that this type is renderer-only.
| DateFieldMetadata
| DateTimeFieldMetadata
| FileFieldMetadata
| StringListFieldMetadata
| CustomFieldMetadata;
packages/apollo-wind/src/components/forms/metadata-form.stories.tsx:1297
- This newly added Storybook prose uses a spaced em dash, which is disallowed for story documentation in this repository. Replace it with a sentence break or colon.
* one row" and `pattern: '\\S'` for "not just whitespace" — no host-side re-implementation.
packages/apollo-wind/src/components/forms/metadata-form.tsx:41
- The advertised controlled-host API is not present in this interface: the PR description lists
values,onValuesChange, externalerrors, anddisableValidation, butMetadataFormPropsonly exposesschema, plugins, submission/presentation options, andcontainer. The new forms README also explicitly says there is no controlled-value prop, so consumers following the PR description cannot compile. Either implement the advertised props or update the PR scope/description to the plugin-only contract.
export interface MetadataFormProps {
packages/apollo-wind/src/components/forms/metadata-form.tsx:372
- This still mounts a default Radix
TooltipProviderwhenever the schema contains a tooltip, even when the form is already inside a host provider. For example, the canvas provider setsdelayDuration={200}andskipDelayDuration={100}, but this nested provider uses Radix defaults and shadows those settings for all tooltips in the form. The comment's claim that host configuration is not overridden is therefore false; reuse/mark an existing provider or otherwise avoid nesting a default provider.
{hasFieldTooltip ? <TooltipProvider>{body}</TooltipProvider> : body}
packages/apollo-wind/src/components/forms/string-list-field.tsx:16
formatTemplatepromises that unknown placeholders remain untouched, buttoken in valuesalso matches inheritedObject.prototypekeys. For a normal record,{{toString}}is replaced with the function source instead of remaining literal. Use an own-property check so only supplied template values are interpolated.
token in values ? String(values[token]) : match
packages/apollo-wind/src/components/forms/validation-converter.ts:60
required: truestill accepts whitespace-only strings whenminLengthis set to at least the whitespace length: the shared emptiness refinement is skipped wheneverconfig.minLengthis truthy, and.min()checks only character count. For example,{ required: true, minLength: 3 }accepts' ', contradicting the whitespace semantics documented above. Keep the required emptiness check for all required strings while preserving the min-length error for shorter values.
if (isStringType(fieldType, customValueType) && !config.minLength) {
schema = schema.refine((value) => !isEmptyFieldValue(value), { message: requiredMessage });
packages/apollo-wind/src/components/forms/validation-converter.ts:36
- The new
customValueTypesupport is only threaded throughMetadataForm's private schema builder. The exportedbuildZodSchemaFromFieldshelper below still accepts novalueTypeand callsvalidationConfigToZodwith only the field type, so custom fields validated through that helper remainz.any()and silently ignorerequired/array constraints. Extend that helper's field shape and forwardvalueType, with a regression test.
customValueType?: CustomValueType
packages/apollo-wind/src/components/forms/validation-converter.ts:49
customValueTypeis threaded into the string/array constraint helpers but not the numeric ones. A{ type: 'custom', valueType: 'number', validation: { min: 10, max: 20, integer: true } }field therefore gets only the basez.number()type check;applyNumberConstraintsstill callsisNumberType(fieldType)withcustomand ignores all numeric constraints. Pass the custom value type through and treat'number'as numeric, with a regression test.
schema = applyStringConstraints(schema, config, fieldType, customValueType);
schema = applyNumberConstraints(schema, config, fieldType);
schema = applyArrayConstraints(schema, config, fieldType, customValueType);
- Files reviewed: 31/31 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate validation, accessibility, designer, serialization, and embedding issues block approval.
Review details
Suppressed comments (14)
Previously missed (5) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/forms/field-renderer.tsx:570
- The new field label is not associated with the radio group. A radiogroup is not labelable, so this label remains visual-only and the group has no accessible name even though
aria-invalidis added; give the label an id and pass it asaria-labelledbyonRadioGroup.
This issue also appears in the following locations of the same file:
- line 625
- line 741
packages/apollo-wind/src/components/forms/form-schema.ts:327
- Adding
StringListFieldMetadatato the publicFieldMetadataunion does not add it toFIELD_TYPE_METADATAor the field configuration schema inform-designer.tsx. The designer's field-type selector therefore cannot create or edit the new field type, despite describing that list as the complete set of supported types. Add the type and its designer settings, or explicitly exclude it from the designer contract.
packages/apollo-wind/src/components/forms/validation-converter.ts:36 - The new
customValueTypeparameter is not threaded intoapplyNumberConstraints, andisNumberTypeonly recognizes the built-in number types. ConsequentlyvalueType: 'number'custom fields silently ignoremin,max,integer,positive, andnegative, despite the new contract saying normal constraints apply to declared custom shapes. Pass the custom type through the number path and cover it with a test.
packages/apollo-wind/src/components/forms/validation-converter.ts:59 - When
required: trueis combined with a positiveminLength, this guard skips the shared emptiness check. For example,{ required: true, minLength: 3 }accepts' 'because it only applies.min(3), so static validation still disagrees with the conditional-required path. Apply theisEmptyFieldValuerefinement regardless ofminLength.
packages/apollo-wind/src/index.ts:40 CustomValueTypeis the new public type used byCustomFieldMetadata.valueType, but this barrel re-exportsStringListFieldMetadatawithout re-exporting the value-shape type. Consumers cannot import the new contract's type from@uipath/apollo-wind(or the forms barrel), leaving them to duplicate the union. Re-export it from both public barrels.
packages/apollo-wind/src/components/forms/field-renderer.tsx:449
- The unconditional
aria-label={field.label}overrides the newhtmlFor/idlabel association and also ignoresfield.ariaLabelfor select fields. A schema-provided accessible name therefore cannot be used, unlike the other controls. Set this fromfield.ariaLabelor omit it so the associated label supplies the name.
aria-label={field.label}
packages/apollo-wind/src/components/forms/field-renderer.tsx:608
- The date picker still has no label/control association or invalid state: this label has no
htmlFor, andDatePickerreceives neither an id noraria-invalidwhile an error is rendered below it. Forward those props to the picker trigger and connect them here so screen readers receive the visible field name and validation state.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:629
- The date-time picker has the same accessibility gap: the newly rendered label is not connected to the picker button, and
aria-invalidis not forwarded even though this branch rendersFormFieldError. Add trigger id/invalid prop support toDateTimePickerand associate the label.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:745
- The slider's visible label is not associated with its Radix slider root, and the root never receives
aria-invalideven though this branch renders a validation error. Give the label an id, pass it viaaria-labelledby, and forward the invalid state to the slider.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/metadata-form.tsx:357
- This condition treats every
<input>other thantype="button"as a single-line text input. Incontainer="div", that also prevents Enter's default keyboard behavior for custom checkbox, radio, file, range, color, reset, submit, and image inputs. Restrict the guard to text-like input types so embedding custom controls does not change their keyboard interaction.
if (target instanceof HTMLInputElement && target.type !== 'button') {
packages/apollo-wind/src/components/forms/metadata-form.tsx:372
- When a schema uses tooltip metadata, this inner
TooltipProviderbecomes the nearest provider and resets any host-supplied delay/hover settings to Radix defaults. The conditional only avoids nesting when there are no tooltips; it does not preserve a host provider’s configuration for the case where tooltips are actually rendered.
return (
<FormProvider {...form}>
{hasFieldTooltip ? <TooltipProvider>{body}</TooltipProvider> : body}
packages/apollo-wind/src/components/forms/schema-serializer.ts:246
- These new string metadata properties are serialized only when truthy, but their renderers use nullish defaults, so an intentional empty
emptyMessage,searchPlaceholder,addItemLabel, orremoveItemAriaLabelis meaningful. Such a schema changes behavior after a serialize/round-trip. Preserve values whenever they are notundefined.
if ('emptyMessage' in field && field.emptyMessage) result.emptyMessage = field.emptyMessage;
packages/apollo-wind/src/components/forms/validation-converter.ts:131
string-listis added as an array schema, but the string constraint path excludes it, so avalidation.pattern(and string length constraints) is silently ignored instead of applying to each row. The new story claims apattern: '\\S'rejects whitespace-only rows, but that behavior cannot work with this converter. Apply the intended per-item constraints or remove the claim and document the limitation.
case 'multiselect':
case 'string-list':
return z.array(z.string());
packages/apollo-wind/src/components/forms/validation-converter.ts:36
MetadataFormnow passesfield.valueTypeto the converter, butbuildZodSchemaFromFieldsstill accepts onlyname/type/validationand calls the converter without that third argument. Callers of this exported helper still validate custom fields asz.any(), sorequiredand list constraints remain no-ops on that path.
fieldType: FieldType,
/**
* Declared shape of a `type: 'custom'` field's value. Custom fields otherwise validate as
* `z.any()`, where `required` and the array constraints are no-ops.
*/
customValueType?: CustomValueType
- Files reviewed: 25/25 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate accessibility, validation, container-behavior, and public API issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (16)
Previously missed (5) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/forms/field-renderer.tsx:570
- The field-level radio label is not connected to the
RadioGroup; the option labels name individual radios, but the group itself has no accessible name from the visible field label. Give theFormFieldLabelan id and pass that id asaria-labelledbyonRadioGroup.
This issue also appears in the following locations of the same file:
- line 625
- line 741
packages/apollo-wind/src/components/forms/validation-converter.ts:49
valueType: 'number'still cannot use the numeric validation metadata becauseapplyNumberConstraintsis called with onlyfieldType, andisNumberTypeonly recognizes'number'/'slider'. Thusmin,max,integer,positive, andnegativeare silently ignored for custom numeric components, despite the newvalueTypecontract saying ordinary constraints apply. Thread the custom value type through the numeric constraint path as was done for strings and arrays.
packages/apollo-wind/src/components/forms/validation-converter.ts:59- When
minLengthis present, the whitespace check is skipped. For example,{ required: true, minLength: 3 }accepts' ', so the resolver still disagrees withisEmptyFieldValuefor this common case. Apply the required emptiness refinement for all required string fields, not only those withoutminLength.
packages/apollo-wind/src/index.ts:40 CustomValueTypeis a new public type used byCustomFieldMetadata, but the package root only re-exportsStringListFieldMetadata. Consumers cannot import the value-shape type from@uipath/apollo-windlike the other form types.
packages/apollo-wind/src/components/forms/metadata-form.stories.tsx:1297- This new Storybook documentation uses an em dash with spaces. Story content in this repository must use a period, colon, or comma instead.
packages/apollo-wind/src/components/forms/field-renderer.tsx:519
- The checkbox branch does not pass
required={required}, so a required checkbox has no required indicator even though its schema enforces requiredness. This makes the label state inconsistent with the other field renderers.
<FormFieldLabel
htmlFor={field.name}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
className="font-normal"
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:610
- Validation errors are rendered for date fields, but this branch never forwards
aria-invalidto the date-picker button, so assistive technology cannot identify the control as invalid. Add an invalid-state prop toDatePickerand pass it fromerrorhere.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
{field.label}
</FormFieldLabel>
packages/apollo-wind/src/components/forms/field-renderer.tsx:631
- The datetime field also renders an error without forwarding
aria-invalidto its picker button, leaving the invalid control unannounced to assistive technology. Add the prop toDateTimePickerand pass it fromerrorhere.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
{field.label}
</FormFieldLabel>
packages/apollo-wind/src/components/forms/field-renderer.tsx:745
- The slider label still has no accessible linkage to the Radix slider, and
erroris rendered below without forwardingaria-invalidto the control. Add a label id, reference it witharia-labelledby, and pass the invalid state so this error-bearing field has the same semantics as the other controls.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:683
- The new string-list Controller field drops
formField.ref. When React Hook Form focuses the first invalid field on submit, it has no element to focus for this field, unlike the native controls rendered above. Forward the ref to the first row (or another focusable target).
<StringListField
field={field}
value={formField.value as string[] | undefined}
onChange={formField.onChange}
onBlur={formField.onBlur}
error={error}
disabled={disabled}
required={required}
/>
packages/apollo-wind/src/components/forms/form-schema.ts:296
CustomValueTypeis a new public type used byCustomFieldMetadata.valueType, but it is not re-exported from either the forms barrel or the root package barrel; the only re-export is invalidation-converter.ts, which is not barrel-exposed. Consumers can use the string literals but cannot import the declared type from the public API. Add it to both type export lists.
/** Value shapes a `type: 'custom'` field can declare so metadata constraints apply to it. */
export type CustomValueType = 'string' | 'number' | 'boolean' | 'string-array';
packages/apollo-wind/src/components/forms/metadata-form.stories.tsx:1297
- The story says
pattern: '\\S'prevents whitespace-only rows, but this schema does not setpattern, and the validator only applies string constraints to scalar string fields, not array elements. The example therefore does not implement the behavior its documentation promises; either add per-item validation or remove that claim.
* The `string-list` field type (repeated multiline rows with Add/Remove) and `tooltip` field
* metadata, validated the way the schema already expresses it: `minItems: 1` for "at least
* one row" and `pattern: '\\S'` for "not just whitespace" — no host-side re-implementation.
packages/apollo-wind/src/components/forms/metadata-form.tsx:301
- This condition treats every input except
buttonas a single-line text control. Incontainer="div", Enter is therefore prevented on native checkbox, radio, file, range, color, submit, and reset inputs, breaking their keyboard activation. Restrict the handler to text-like input types or explicitly exclude those controls.
if (target instanceof HTMLInputElement && target.type !== 'button') {
event.preventDefault();
}
packages/apollo-wind/src/components/forms/string-list-field.tsx:97
- The new string-list label has no
htmlFor, and its row textareas have no ids. Although each row has anaria-label, the visible field label is not programmatically associated with any of the repeated controls, unlike the other renderer fields. Add an explicit group/row labeling scheme so the label remains associated when rows are added or removed.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/string-list-field.tsx:69
- The out-of-band resync does not preserve row identity when an item is removed or reordered externally. For example, changing
['a', 'b', 'c']to['a', 'c']truncates[A, B, C]to[A, B], socreusesb's key and a focused textarea can be associated with a different row. Since plugins can callcontext.form.setValue, either track row identity with the values or deliberately reset keys for external array replacements instead of truncating by length.
// Stable per-row ids: without them, removing index N would reuse the DOM node of a
// surviving sibling and swap a focused textarea's content with someone else's. Resync
// during render (React's official pattern) when the `items` array length changes
// out-of-band; the local add/remove handlers below already mutate ids in lockstep.
const [rowIds, setRowIds] = useState<string[]>(() => items.map(() => crypto.randomUUID()));
const [prevLength, setPrevLength] = useState(items.length);
if (prevLength !== items.length) {
setPrevLength(items.length);
setRowIds((prev) =>
prev.length < items.length
? [
...prev,
...Array.from({ length: items.length - prev.length }, () => crypto.randomUUID()),
]
: prev.slice(0, items.length)
);
}
packages/apollo-wind/src/components/forms/validation-converter.ts:94
- The existing
ValidationConfigcontract documents expressions such asvalue.length > otherField, but this evaluates against an object containing onlyvalue. Any reference to another form field resolves toundefinedand can make an otherwise valid value fail permanently. Evaluate custom validation with the full form values, or explicitly narrow the public contract and documentation.
const result = RulesEngine.tryEvaluateExpression(expression, { value });
- Files reviewed: 25/25 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Outstanding moderate review findings must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (14)
Previously missed (1) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/forms/field-renderer.tsx:563
- The visible group label is not connected to the
RadioGroup: it has neitherhtmlFornor an id referenced byaria-labelledby, and the group itself has no accessible name. The option labels name individual radios but do not name the radiogroup; add an explicit group-label association.
This issue also appears in the following locations of the same file:
- line 625
- line 741
packages/apollo-wind/src/components/forms/field-renderer.tsx:608
- The date branch still leaves
FormFieldLabelwithouthtmlFor, andDatePickerreceives noid; its button is therefore not associated with the visible field label. Add id support toDatePickerand passfield.name(or otherwise wire the button's accessible name) so the renderer-wide label association also covers dates.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:629
- The datetime branch has the same gap:
FormFieldLabelhas nohtmlFor, andDateTimePickerreceives noid, so the visible field label does not name or focus its button. Add id support to the picker and passfield.name.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:745
SliderFieldalso renders a label with nohtmlFor, while the Radix slider root has no matching id. Its focusable thumb consequently has no accessible name from the visible field label; passfield.nameas the slider id and target it from the label.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:629
- This branch also renders
FormFieldErrorwithout forwardingaria-invalidto the datetime picker button.DateTimePickerhas no invalid-state prop, so failed validation is not exposed to assistive technology or matching control styling; thread the prop through the picker and pass it here.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:608
- This branch still renders
FormFieldErrorwithout forwardingaria-invalidto the date picker button.DatePickerhas no invalid-state prop, so a failed date validation is not exposed to assistive technology or the control's invalid styling; add the prop through the picker and pass it here.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/index.ts:41
CustomValueTypeis part of the new publicCustomFieldMetadata.valueTypeAPI, but this public forms barrel exportsStringListFieldMetadataand omitsCustomValueType. Consumers importing schema types from@uipath/apollo-wind/components/formscannot import the value-type union; add it to this barrel and the root barrel as well.
StringListFieldMetadata,
packages/apollo-wind/src/components/forms/metadata-form.stories.tsx:1297
- This new Storybook documentation sentence uses a spaced em dash. Replace it with a sentence break so the story copy follows the repository's punctuation convention.
* one row" — no host-side re-implementation.
packages/apollo-wind/src/components/forms/metadata-form.tsx:299
- This guard treats every input except
type="button"as a single-line text control. Incontainer="div", Enter on file, checkbox, radio, submit, reset, and similar inputs is prevented, blocking their native keyboard activation. Restrict the swallow to text-like input types instead.
if (target instanceof HTMLInputElement && target.type !== 'button') {
packages/apollo-wind/src/components/forms/metadata-form.tsx:388
- Conditioning this provider on schema metadata does not preserve an ancestor provider's settings. If a host wraps
MetadataFormin<TooltipProvider delayDuration={300}>, this nested default provider becomes the nearest context for everyInfoTooltipand resets the host's delay/skip-delay behavior. Expose/configure the provider or avoid nesting rather than silently overriding the host.
{hasFieldTooltip ? <TooltipProvider>{body}</TooltipProvider> : body}
packages/apollo-wind/src/components/forms/string-list-field.tsx:97
StringListFieldrenders a visibleFormFieldLabelwithouthtmlFor, while its row textareas have noid. The label therefore cannot focus or associate with any control, unlike the other renderer branches; give the first row a stable id and target it while retaining the per-row accessible names.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/string-list-field.tsx:68
- The out-of-band length reconciliation only truncates or appends ids by position. If a host uses the documented
context.form.setValueseam to remove the first row, surviving values inherit the deleted row's keys, so React can reuse the wrong textarea DOM state and move focus/cursor state to a different value. Reconcile external replacements by identity where possible or regenerate ids rather than assuming a length change is an append/tail removal.
setRowIds((prev) =>
prev.length < items.length
? [
...prev,
...Array.from({ length: items.length - prev.length }, () => crypto.randomUUID()),
]
: prev.slice(0, items.length)
);
packages/apollo-wind/src/components/forms/validation-converter.ts:97
- This refinement is attached to the field schema regardless of visibility.
FormFieldRendererunmounts hidden controllers whileuseFormkeeps their values, so a hidden field with a retained value can failValidationConfig.customand block submission even though the form's dynamic validation explicitly skips hidden fields. Gate this check by visibility or omit hidden values from the resolver.
return schema.superRefine((value: unknown, ctx: z.RefinementCtx) => {
const result = RulesEngine.tryEvaluateExpression(expression, { value });
if (!result.ok) return;
if (!result.value) {
ctx.addIssue({ code: 'custom', message });
packages/apollo-wind/src/index.ts:40
- The root barrel now exposes
StringListFieldMetadatabut still omits the newCustomValueTypeunion used byCustomFieldMetadata.valueType. A consumer importing public schema types from@uipath/apollo-windcannot name that value shape; re-export it alongside the other form schema types.
StringListFieldMetadata,
- Files reviewed: 25/25 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved accessibility, API export, validation, serialization, and integration issues remain.
Review details
Suppressed comments (20)
Previously missed (5) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/forms/field-renderer.tsx:571
- The radio group's visible field label is not associated with the
radiogroup: no label id is assigned andRadioGrouphas noaria-labelledby. The individual options are named, but the group-level field name is lost for assistive technology.
packages/apollo-wind/src/components/forms/field-renderer.tsx:631 - The datetime field has the same gap:
FormFieldErroris rendered, butDateTimePickerreceives neither an id/accessible label association noraria-invalid. A screen reader cannot relate the visible field label or invalid state to this trigger; add the corresponding picker props and renderer wiring.
packages/apollo-wind/src/components/forms/field-renderer.tsx:747 - The slider label is still not associated with the Radix slider, and this branch renders an error without forwarding
aria-invalid. Add a label id and passaria-labelledbyplus the invalid state to the slider root so both the field name and validation state are exposed.
packages/apollo-wind/src/components/forms/schema-serializer.ts:253 - These new truthiness checks drop valid explicit empty-string overrides for
emptyMessage,searchPlaceholder,addItemLabel, andremoveItemAriaLabelduring serialization, even though the renderer treats''as a supplied value via??. That breaks the round-trip guarantee for the new metadata; check againstundefinedinstead.
packages/apollo-wind/src/components/forms/validation-converter.ts:152 CustomValueTypeaccepts'number', but this only changes the base schema.applyNumberConstraintsstill checksisNumberType(fieldType)without the custom value type, so a custom field declared asvalueType: 'number'silently ignoresmin,max,integer,positive, andnegative. ThreadcustomValueTypethrough the numeric helper just as the string and array helpers do.
packages/apollo-wind/src/components/forms/README.md:16
- The new ownership contract correctly says custom components come from a
FormPlugin, but the existing custom-components example later in this README still passes the removedcustomComponentsprop directly toMetadataForm. That example is now invalid TypeScript/API guidance; update it to register the component throughplugins={[{ components: ... }]}.
| Observe or drive values | a `FormPlugin`: `onValueChange` to read, `context.form.setValue` to write |
| Own a validation rule the schema cannot express | a plugin calling `context.form.setError` / `clearErrors` |
| Replace the form's state wholesale (undo/redo, switching entity) | remount with a new `key` |
packages/apollo-wind/src/components/forms/field-renderer.tsx:610
- This date field still renders a
FormFieldLabelwithouthtmlFor, whileDatePicker's trigger has no correspondingidand only gets a generic accessible name. The visible field label is therefore not programmatically associated with the control; add id/label plumbing (and forwardaria-invalid, since this branch also rendersFormFieldError).
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
{field.label}
</FormFieldLabel>
packages/apollo-wind/src/components/forms/field-renderer.tsx:745
SliderFieldstill rendersFormFieldErrorbelow the control, but it never forwardsaria-invalidto the slider. Validation and plugin errors are therefore not exposed as an invalid state to assistive technology; pass the state through to the actual slider/thumb.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:608
- This label is not associated with the date control.
DatePickergives its trigger anaria-labelsuch as “Pick a date”, which overrides the visible label for assistive technology, and this renderer supplies neitherhtmlFornor a matching control id. Add id/labelledby support to the picker and pass it here so multiple date fields remain distinguishable.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:629
- The datetime label has the same missing association:
DateTimePicker's trigger is rendered without an id and thisFormFieldLabelhas nohtmlFor. Its placeholder/value text is not a reliable field name when a form contains more than one datetime field, so add an id/labelledby path through the picker.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:608
- The date branch also renders
FormFieldErrorbut does not forwarderrorasaria-invalidto the DatePicker trigger. Add an invalid-state prop through DatePicker so schema/plugin errors are announced on the control, not only in the live message below.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:629
- The datetime branch has the same gap: it displays
FormFieldErrorbut never marks the DateTimePicker trigger invalid. Forwardaria-invalidthrough DateTimePicker so assistive technology gets the control state as well as the message.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/form-schema.ts:331
string-listis now part of the publicFieldMetadataunion and has a renderer, butFormDesigner'sFIELD_TYPE_METADATAstill omits it and its configuration/sync flow has no list settings. The exported designer therefore cannot create this newly supported field type; add the type to the designer flow or explicitly exclude it from the supported union.
| StringListFieldMetadata
packages/apollo-wind/src/components/forms/form-schema.ts:300
CustomValueTypeis a new public part ofCustomFieldMetadata.valueType, but it is not re-exported from eithercomponents/forms/index.tsor the package root. Consumers can see the property throughFieldMetadatabut cannot import the named type from the public API; add it to the public type barrels.
/** Value shapes a `type: 'custom'` field can declare so metadata constraints apply to it. */
export type CustomValueType = 'string' | 'number' | 'boolean' | 'string-array';
packages/apollo-wind/src/components/forms/index.ts:42
- The newly public
CustomValueTypeis omitted from this forms barrel even thoughCustomFieldMetadataexposesvalueType. Consumers importing forms from@uipath/apollo-wind/formstherefore cannot type that property; re-exportCustomValueTypehere.
export type {
CustomFieldComponentProps,
DataSource,
FieldCondition,
FieldMetadata,
FieldOption,
FieldRule,
FieldType,
FormAction,
FormContext,
FormPlugin,
FormSchema,
FormSection,
FormStep,
StringListFieldMetadata,
} from './form-schema';
packages/apollo-wind/src/components/forms/metadata-form.stories.tsx:1297
- Storybook documentation in this added block uses an em dash, which is disallowed for story copy in this repository. Replace it with a colon or a new sentence.
* one row" — no host-side re-implementation.
packages/apollo-wind/src/components/forms/metadata-form.tsx:388
- This conditional wrapper still overrides an ancestor
TooltipProviderwhenever the schema has a tooltip, because Radix uses the nearest provider context. A host configured withdelayDuration,skipDelayDuration, ordisableHoverableContentwill silently get the defaults insideMetadataForm; make the provider configurable/opt-out or otherwise avoid nesting it.
{hasFieldTooltip ? <TooltipProvider>{body}</TooltipProvider> : body}
packages/apollo-wind/src/components/forms/schema-serializer.ts:227
- The new tooltip metadata is also serialized with truthiness checks, so an explicitly empty
tooltiportooltipAriaLabelis dropped on a schema round trip. This changes the presence-sensitive rendering behavior; preserve defined values with!== undefined, as for the other string metadata.
if (field.tooltip) result.tooltip = field.tooltip;
if (field.tooltipAriaLabel) result.tooltipAriaLabel = field.tooltipAriaLabel;
packages/apollo-wind/src/components/forms/string-list-field.tsx:48
validation.maxItemsis already enforced by the resolver, but the new control only reads the separate top-levelfield.maxItems. A schema that puts the generic constraint undervalidationcan therefore add unlimited rows and discover the violation only on submit. Use the validation value as a fallback while keeping the top-level UI cap as the override.
const maxItems = field.maxItems ?? Number.POSITIVE_INFINITY;
packages/apollo-wind/src/index.ts:40
CustomValueTypeis a new public schema type used byCustomFieldMetadata, but it is omitted from both this root barrel andcomponents/forms/index.ts. Consumers can use the literal property but cannot import the type from either public entry point, so the typed-custom-field API is incomplete; export it from both barrels.
FormSchema,
FormSection,
FormStep,
StringListFieldMetadata,
- Files reviewed: 25/25 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate accessibility, serialization, focus-management, provider, and public-export issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (14)
Previously missed (5) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/forms/field-renderer.tsx:571
- The field label is still not programmatically associated with this radio group.
aria-invalidnames the group's state, but without a label id andaria-labelledbythe group has no accessible group name; the option labels only name individual radios. Give theFormFieldLabelan id and reference it fromRadioGroup.
packages/apollo-wind/src/components/forms/field-renderer.tsx:629 - The datetime field has the same missing control relationship as the date field: its visible label has no
htmlFor/accessible-labelledby relationship, and its button is not marked invalid whenFormFieldErrorrenders. Thread an id and invalid state throughDateTimePickerand associate it here.
packages/apollo-wind/src/components/forms/field-renderer.tsx:747 - This slider still renders a visible label and
FormFieldErrorwithout giving the slider an accessible label relationship or anaria-invalidstate. The other updated controls now expose these states, but a failing slider remains unnamed/valid-looking to assistive technology. Associate a label id with the slider and forward the error state.
packages/apollo-wind/src/components/forms/metadata-form.tsx:388 - When a schema contains tooltip metadata, this always inserts a new default
TooltipProvideraround the form. If the host already provides one with customdelayDuration,skipDelayDuration, ordisableHoverableContent, the nested provider shadows those settings for the form's tooltips. Reuse the host provider when possible or expose a way for the host to configure the provider.
packages/apollo-wind/src/components/forms/schema-serializer.ts:253 - These truthiness checks drop explicitly configured empty strings during serialization, even though the corresponding renderers use
??and therefore treat an empty search placeholder, empty message, or add/remove label as a valid override. A schema round-trip consequently changes those values; preserve them with!== undefinedchecks.
packages/apollo-wind/src/components/forms/field-renderer.tsx:608
- This date field's visible label is not associated with the
DatePickerbutton, and the button receives noidoraria-invalidwhen the renderedFormFieldErroris present. BecauseDatePickerwraps its control internally, add those props to that component and pass the label/error relationship here (the datetime branch has the same omission).
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:679
- The new composite field drops
formField.refwhen renderingStringListField. React Hook Form's default submit-error behavior uses that ref to focus the first invalid control, so arequired/minItemsfailure will show an error but leave focus wherever it was. Forward the controller ref to the first row textarea (or an equivalent focusable list target).
case 'string-list':
return (
<StringListField
field={field}
value={formField.value as string[] | undefined}
onChange={formField.onChange}
onBlur={formField.onBlur}
packages/apollo-wind/src/components/forms/schema-serializer.ts:227
- The new tooltip metadata is not fully round-trippable: an explicitly empty string is dropped here, while
FormFieldLabeltreatstooltip: ''as present and renders a tooltip trigger. Serializing and deserializing the schema therefore changes the rendered result; use anundefinedcheck for both tooltip properties.
if (field.tooltip) result.tooltip = field.tooltip;
if (field.tooltipAriaLabel) result.tooltipAriaLabel = field.tooltipAriaLabel;
packages/apollo-wind/src/components/forms/string-list-field.tsx:24
- This new field is wired through
Controller, but it never receives or forwards the controller ref, so React Hook Form cannot focus the first invalid row whenshouldFocusErrorruns on submit. Thread a focus ref throughStringListFieldto the first textarea, as the other controlled field renderers do.
export interface StringListFieldProps {
field: StringListFieldMetadata;
value: string[] | undefined;
onChange: (value: string[]) => void;
/** Forwarded to every row so blur-mode validation and touched state work. */
packages/apollo-wind/src/components/forms/string-list-field.tsx:110
- The new field ignores the inherited
ariaLabelmetadata and always names rows fromfield.label, so schemas that provide an accessible-name override get different behavior forstring-listthan for the other renderer branches. Use the override as the row label prefix.
aria-label={`${field.label} ${index + 1}`}
packages/apollo-wind/src/components/forms/string-list-field.tsx:103
- The new field also ignores the inherited
placeholdermetadata, unlike the other renderer branches, so a schema-provided row hint silently disappears. Passfield.placeholderto each row textarea.
<Textarea
packages/apollo-wind/src/components/forms/string-list-field.tsx:97
- The new list label has neither
htmlFornor an associated rowid, so clicking the visible “Phrases” label cannot focus a control and the label/control association promised by the renderer is missing for this field type. Give the list an explicit grouping/label relationship (for example, a fieldset/legend or a labelled first-row target).
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/string-list-field.tsx:16
formatTemplatepromises to leave unknown tokens untouched, buttoken in valuesalso matches inherited keys such astoStringandconstructor. A template containing{{toString}}is therefore replaced with the function's source instead of remaining unchanged. Check own properties before interpolating.
return template.replace(/\{\{(\w+)\}\}/g, (match, token: string) =>
token in values ? String(values[token]) : match
packages/apollo-wind/src/index.ts:40
CustomValueTypeis part of the new public custom-field contract, but it is not exported from either@uipath/apollo-windor the/formsbarrel. Consumers can use literal values throughFieldMetadata, but cannot import the shared type for plugin/schema helpers. Add it to the public type exports alongsideStringListFieldMetadata.
StringListFieldMetadata,
- Files reviewed: 25/25 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Stateful chat renderers can remount, and accessibility and numeric-validation issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
packages/apollo-wind/src/components/forms/field-renderer.tsx:450
- The new
id/htmlForassociation is overridden by the unconditionalaria-label={field.label}.aria-labeltakes precedence over a native label, so assistive technology will not use the visible label and an explicitfield.ariaLabelis ignored. Only provide an aria label for the explicit metadata override, or omit it when the trigger has an associated label.
<SelectTrigger
id={field.name}
aria-label={field.label}
aria-invalid={error ? true : undefined}
packages/apollo-wind/src/components/forms/field-renderer.tsx:608
- This label still has no association with the date button:
DatePickeris rendered without an id or labelled-by relationship, so a screen reader announces only its placeholder/selected date rather than the field label. Add an id/aria-labelledbypath to the picker and use it here; the same gap exists in the datetime case below.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
- Files reviewed: 27/27 changed files
- Comments generated: 6
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings remain across accessibility, validation, serialization, and integration behavior.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (11)
packages/apollo-wind/src/components/forms/field-renderer.tsx:608
- The date field's visible label still has no
htmlFor, whileDatePicker's trigger has no id or labelled-by prop. As a result, assistive technology receives only the placeholder/selected-date aria label, not the field label. Add id/accessible-name support to the picker and wire it here; the datetime case below has the same gap.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:629
- The datetime field has the same label association gap:
FormFieldLabelhas nohtmlFor, andDateTimePickerexposes no id/accessible-name prop for its trigger. Screen readers therefore do not get the visible field label as the control name; extend the picker API and connect it here.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:563
- The radio group label is still not associated with the group: the label has no id and the
RadioGroupbelow has noaria-labelledby/accessible name. Individual options are named, but assistive technology does not announce what the group represents. Give the group an accessible name fromfield.label.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:747
- The slider branch has the same accessibility gap:
FormFieldLabelis not associated with the Radix slider, and the slider receives neither an accessible name noraria-invalid, even though a validation error is rendered below. Give the label an id and passaria-labelledby, an id, and the invalid state toSlider.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
{field.label}
</FormFieldLabel>
packages/apollo-wind/src/components/forms/field-renderer.tsx:518
- Unlike the other required-aware renderers, this checkbox label drops
required={required}, so a schema withvalidation.requiredgets no required indicator even thoughfieldState.requiredis computed and passed into this renderer. Forward the flag when composing the label.
<FormFieldLabel
htmlFor={field.name}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
className="font-normal"
packages/apollo-wind/src/components/forms/field-renderer.tsx:354
- Using the raw field name as a document-wide
idis unsafe when twoMetadataForminstances use the same schema or common names: duplicate IDs makehtmlForand assistive-technology references resolve to the wrong form. Generate an instance-scoped control ID (and reuse it for the label) instead of assumingfield.nameis globally unique.
id={field.name}
packages/apollo-wind/src/components/forms/metadata-form.stories.tsx:1297
- This new Storybook documentation comment uses an em dash with spaces, which violates the repository's Storybook copy convention. Replace it with a colon or a new sentence.
* one row" — no host-side re-implementation.
packages/apollo-wind/src/components/forms/metadata-form.tsx:388
- This always nests a new Radix
TooltipProviderinside any provider supplied by the host, resetting host-configureddelayDuration/skipDelayDurationto defaults whenever a schema has tooltip metadata. Canvas already uses an explicit provider/marker pattern (packages/apollo-react/src/canvas/components/CanvasTooltip.tsx:21-30) to avoid this shadowing; reuse an ancestor or make provider ownership explicit here.
return (
<FormProvider {...form}>
{hasFieldTooltip ? <TooltipProvider>{body}</TooltipProvider> : body}
packages/apollo-wind/src/components/forms/schema-serializer.ts:246
- These truthiness checks discard valid empty-string metadata during serialization.
emptyMessage,searchPlaceholder,addItemLabel, andremoveItemAriaLabelare consumed with??in the renderer, so explicitly setting one to''is meaningful but changes after a serialize/round-trip. Preserve values with!== undefinedchecks.
if ('emptyMessage' in field && field.emptyMessage) result.emptyMessage = field.emptyMessage;
packages/apollo-wind/src/components/forms/string-list-field.tsx:110
StringListFieldMetadatainherits the commonariaLabel, but every row hardcodesfield.label, so a schema-provided accessible name is ignored for the new control. Use the override when naming each row, falling back to the visible label.
aria-label={`${field.label} ${index + 1}`}
packages/apollo-wind/src/components/forms/validation-converter.ts:49
- Passing
customValueTypehere only reaches string/array handling;applyNumberConstraintsstill receives justfieldType, andisNumberTypeonly recognizes the built-innumber/slidertypes. Atype: 'custom', valueType: 'number'field therefore silently ignoresmin,max,integer,positive, andnegative, despite the new contract saying normal metadata constraints apply. Thread the value type through the number helper and add a regression test.
schema = applyStringConstraints(schema, config, fieldType, customValueType);
schema = applyNumberConstraints(schema, config, fieldType);
schema = applyArrayConstraints(schema, config, fieldType, customValueType);
- Files reviewed: 25/25 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved accessibility, export, provider, serialization, labeling, and custom value-type issues remain.
Review details
Suppressed comments (8)
packages/apollo-wind/src/components/forms/field-renderer.tsx:613
- The date branch still leaves the visible label unassociated with its trigger: unlike datetime/slider, the label has no
htmlFor/id relationship, whileDatePickersupplies only its genericaria-label(Pick a date/Selected date). A screen reader therefore announces the generic date control rather than the schema field label. It also rendersFormFieldErrorwithout forwardingaria-invalid, so an invalid date is not exposed on the control. Add id/aria-labelledbyand invalid-state support toDatePicker(avoiding an overridingaria-label) and pass them here.
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
{field.label}
packages/apollo-wind/src/components/forms/index.ts:28
- The package exposes
CustomValueTypefromsrc/index.ts, but the public./components/formsbarrel (an exports-map entry) omits it. Consumers using that subpath cannot type the newvalueTypemetadata without reaching into an internal module; add it to this type export list.
CustomFieldComponentProps,
packages/apollo-wind/src/components/forms/metadata-form.stories.tsx:1297
- This new Storybook documentation line uses the spaced em dash form
—. Story copy in this repository avoids that form; replace it with a period or comma.
* one row" — no host-side re-implementation.
packages/apollo-wind/src/components/forms/metadata-form.tsx:388
- Whenever a schema contains tooltip metadata, this creates a new nearest Radix
TooltipProvidereven when the form is already under a host provider. Its default settings then override host policies such asdelayDurationandskipDelayDuration(the repository has hosts usingdelayDuration={300}), so embedded form tooltips no longer follow the host configuration. Avoid nesting when a provider is supplied or expose an explicit provider/configuration seam.
{hasFieldTooltip ? <TooltipProvider>{body}</TooltipProvider> : body}
packages/apollo-wind/src/components/forms/schema-serializer.ts:227
- These new metadata fields are serialized only when truthy, so a schema with
tooltip: ''or an explicitly emptytooltipAriaLabelchanges when serialized.FormFieldLabeltreats an empty tooltip as present and preserves an empty aria label, so this is not a no-op; preserve defined values with!== undefined.
if (field.tooltip) result.tooltip = field.tooltip;
if (field.tooltipAriaLabel) result.tooltipAriaLabel = field.tooltipAriaLabel;
packages/apollo-wind/src/components/forms/schema-serializer.ts:250
- The same round-trip loss applies to the new
emptyMessage,searchPlaceholder,addItemLabel, andremoveItemAriaLabelsettings: empty strings are allowed by their types and are meaningful because the controls default only when the value is undefined. The truthy checks drop them, so serialize these fields when!== undefined.
if ('emptyMessage' in field && field.emptyMessage) result.emptyMessage = field.emptyMessage;
if ('searchPlaceholder' in field && field.searchPlaceholder) {
result.searchPlaceholder = field.searchPlaceholder;
}
if ('addItemLabel' in field && field.addItemLabel) result.addItemLabel = field.addItemLabel;
packages/apollo-wind/src/components/forms/string-list-field.tsx:106
- The new list label is not associated with any row: it has no
htmlFor, and the row textareas receive neither anidnor the field name. Clicking the visible label cannot focus a row, and the field-level label is not programmatically associated; give rows stable ids and associate the label, or use fieldset/legend semantics for the group.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/validation-converter.ts:36
customValueTypeis threaded throughMetadataForm, but the exportedbuildZodSchemaFromFieldshelper still accepts only{ name, type, validation }and invokes this converter without the value type. Consumers using that helper therefore still getz.any()for custom fields, sorequired, array, and numeric constraints remain no-ops despite the new custom-field contract. AddvalueTypeto that helper's field input and forward it.
/**
* Declared shape of a `type: 'custom'` field's value. Custom fields otherwise validate as
* `z.any()`, where `required` and the array constraints are no-ops.
*/
customValueType?: CustomValueType
- Files reviewed: 26/26 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved accessibility, export, serialization, documentation, and tooltip-provider issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (11)
packages/apollo-wind/src/components/forms/README.md:21
- The new ownership contract says hosts use plugins, but the existing custom-field example later in this README still passes a
customComponentsprop toMetadataForm; that prop is not inMetadataFormPropsand does not compile. Update the example to useFormPlugin.componentsso the documentation matches the new contract.
There is deliberately no controlled-value prop. A second source of truth for values means
two things can disagree about what the user typed, and every consumer then reimplements the
reconciliation. `context.form` is the full `UseFormReturn`, so anything RHF can do, a plugin
can do.
packages/apollo-wind/src/components/forms/field-renderer.tsx:614
- The date branch remains unnamed by its visible field label and does not expose resolver errors on the date button: unlike the datetime branch,
FormFieldLabelhas nohtmlForandDatePickerreceives noidoraria-invalid. ExtendDatePickerto forward these attributes to its trigger, then pass them here.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
{field.label}
</FormFieldLabel>
packages/apollo-wind/src/components/forms/field-renderer.tsx:691
- The new string-list rows are named only with
${field.label} ${index + 1}, so the commonBaseFieldMetadata.ariaLabeloverride is ignored. Unlike the regular input/file paths, a string-list has no other naming mechanism for each textarea; pass the override through and use it as the row-label prefix.
case 'string-list':
return (
<StringListField
field={field}
inputRef={formField.ref as React.Ref<HTMLTextAreaElement>}
value={formField.value as string[] | undefined}
onChange={formField.onChange}
onBlur={formField.onBlur}
error={error}
disabled={disabled}
required={required}
/>
packages/apollo-wind/src/components/forms/field-renderer.tsx:451
- The renderer now adds
id/htmlFor, but the hard-codedaria-label={field.label}takes precedence over that association and ignores the existingfield.ariaLabeloverride. This makes the schema's custom accessible label ineffective for select fields; passfield.ariaLabel(or omit the attribute when absent) so the label association supplies the default.
<SelectTrigger
id={field.name}
aria-label={field.label}
aria-invalid={error ? true : undefined}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:495
- Unlike
InputandFileUpload, this new multiselect path never forwardsfield.ariaLabelto the trigger. Schemas that provide an explicit accessible name therefore still get only the visible label/default generated name; add anaria-labelprop toMultiSelectand pass this override through.
<MultiSelect
id={field.name}
selected={(formField.value as string[]) || []}
onChange={formField.onChange}
options={options.map((opt) => ({
label: opt.label,
value: String(opt.value),
}))}
disabled={disabled}
placeholder={field.placeholder || 'Select items...'}
emptyMessage={field.emptyMessage ?? 'No items found.'}
searchPlaceholder={field.searchPlaceholder ?? 'Search...'}
maxSelected={field.maxSelected}
aria-invalid={error ? true : undefined}
packages/apollo-wind/src/components/forms/metadata-form.stories.tsx:1297
- This newly added Storybook documentation uses a spaced em dash. Story copy in
.stories.tsxmust not use—; replace it with a period or colon so the new docs follow the repository's Storybook documentation convention.
* one row" — no host-side re-implementation.
packages/apollo-wind/src/components/forms/metadata-form.tsx:388
TooltipProvideris nested whenever the schema has tooltip metadata. Radix resolves the nearest provider, so an outer host provider's delay/skip configuration is overridden by this default provider; conditional mounting does not avoid that override. Expose provider configuration/opt-out or otherwise preserve the host provider settings.
{hasFieldTooltip ? <TooltipProvider>{body}</TooltipProvider> : body}
packages/apollo-wind/src/components/forms/schema-serializer.ts:250
- The new text metadata uses nullish defaults in the renderer, so empty strings are meaningful overrides (for example, an intentionally blank search or add label). These truthy checks omit those values and break the documented JSON round trip; use
!== undefinedfor each optional string.
if ('emptyMessage' in field && field.emptyMessage) result.emptyMessage = field.emptyMessage;
if ('searchPlaceholder' in field && field.searchPlaceholder) {
result.searchPlaceholder = field.searchPlaceholder;
}
if ('addItemLabel' in field && field.addItemLabel) result.addItemLabel = field.addItemLabel;
packages/apollo-wind/src/components/forms/schema-serializer.ts:227
- These truthy checks drop an explicitly supplied empty
tooltiportooltipAriaLabel, even thoughFormFieldLabeltreats an empty string as supplied and the serializer promises a round trip. Use!== undefinedso serialization does not change the field's tooltip behavior or accessible name.
if (field.tooltip) result.tooltip = field.tooltip;
if (field.tooltipAriaLabel) result.tooltipAriaLabel = field.tooltipAriaLabel;
packages/apollo-wind/src/components/forms/string-list-field.tsx:108
- The new repeated rows are not associated with the visible field label: this label has no
htmlFor, and the row textareas below have noid(theiraria-labels do not make clicking the visible label focus a row). Give the first row a stable id and point the label at it so the new field follows the renderer's label/control association contract.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
{field.label}
</FormFieldLabel>
packages/apollo-wind/src/components/forms/string-list-field.tsx:114
- When a required string-list starts empty,
items.maprenders no textarea, soinputRefis never attached. React Hook Form's defaultshouldFocusErrortherefore cannot move focus to the invalid field after submit, leaving focus at the submit/add controls despite this component's ref contract; render or target a focusable empty-state control.
{items.map((item, index) => (
<div key={rowIds[index] ?? index} className="flex items-start gap-2">
<Textarea
ref={index === 0 ? inputRef : undefined}
value={item}
- Files reviewed: 26/26 changed files
- Comments generated: 1
- Review effort level: Lite
…airs Generic forms/ and ui/ enablers extracted from the guardrails work. The guardrails domain family itself lives in apollo-react (#1138, stacked on this). Squashed from 20 commits at review request: most were iterations on each other, and two cancelled out entirely (the controlled-host seam was added and then removed during review), which would otherwise have cut a major release for props that no release ever shipped. New in the forms layer: - `string-list` field type — repeated rows with Add/Remove (`maxItems`, `maxLength`, stable row ids), zod conversion, and `formatTemplate`. Not yet offered by FormDesigner; the reason is documented at FIELD_TYPE_METADATA. - `tooltip` / `tooltipAriaLabel` field metadata rendered by `FormFieldLabel`, plus textarea `minRows`/`maxLength` and multiselect `emptyMessage` / `searchPlaceholder`. - `container: 'form' | 'div'` for embedding inside a host's own chrome: submit actions become plain buttons wired to the form's handler, and Enter is swallowed for single-line inputs so it cannot trigger the host form's implicit submission. - `InfoTooltip` promoted into `components/ui`, with `FormFieldLabel` owning the composition so call sites stop reassembling label + indicator + tooltip. - `MetadataFormProps`, `useWatch` and `CustomValueType` exported, so cross-package custom fields share one react-hook-form instance and can declare their value shape. Repaired — declared in the schema contract but never implemented: - `ValidationConfig.custom` was typed, documented and serialized, but the converter never read it. Now enforced. `RulesEngine.tryEvaluateExpression` distinguishes "the evaluator threw" from "the expression returned falsey", so an expression it cannot handle is not enforced rather than pinning the field permanently invalid. Its scope is documented as single-field; cross-field logic belongs in `rules`. - `FormPlugin.components` was typed and never read, which is why hosts registered asynchronously and missed the first paint. - `plugin.onValueChange` sat behind a mount-lifetime gate that could swallow a plugin's first keystroke. - Custom fields validated as `z.any()`, where `required` and `minItems` are no-ops; they can now declare a `valueType`. Correctness and accessibility: - A required string field rejected `' '` on one path and accepted it on the other. Both now use `isEmptyFieldValue`, as a `.refine` rather than `.trim().min(1)`, which would mutate the submitted value. Behaviour change for anyone who relied on whitespace satisfying `required`, called out in the PR description. - `required` was defeated by an explicit `minItems: 0`. - `aria-invalid` is forwarded by every control that can render an error, with matching invalid styling on Select and Textarea. Radix renders the slider thumb and the datetime trigger as the interactive element, so both take the invalid state and an `aria-labelledby` explicitly — a neighbouring <label> reaches neither — and the radio group had no accessible name of its own. - `StringListField` receives the Controller ref and attaches it to the first row, so react-hook-form's `shouldFocusError` can reach it on a failed submit like every other built-in control. - A custom field declaring `valueType: 'number'` now gets the numeric constraints: `applyNumberConstraints` only saw `fieldType`, so `min`, `max` and `integer` were silently dropped and `{ min: 1 }` still accepted 0. `buildZodSchemaFromFields` accepts and forwards `valueType` too — it is exported, and without it a whole-form schema built through that helper validated every typed custom field as `z.any()`. - Label/control association (`htmlFor` + `id`) across the renderer. - The schema serializer carries the new field metadata, so a round-trip no longer drops it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate findings remain in accessibility, tooltip-provider handling, serialization, string-list templating, and required-value coercion; documentation and test follow-ups also remain.
Review details
Suppressed comments (8)
packages/apollo-wind/src/components/forms/README.md:32
- The new public
string-listfield and itsmaxItems/maxLength/minRowsoptions are not added to the established Field Types table later in this README (and neither are the newcontainer/tooltip APIs). Add those entries so the package documentation exposes the APIs introduced by this change.
**Validation that looks like it needs a host usually does not.** `required`, `pattern`,
`minItems`/`maxItems`, `minLength`/`maxLength`, `min`/`max` and a jsep `custom` expression all
live in `validation`. Custom components participate too, once the field declares its
`valueType` — without it a `type: 'custom'` field validates as `z.any()`, where `required` is
a no-op.
packages/apollo-wind/src/components/forms/field-renderer.tsx:612
- The date branch still renders
DatePickerwithout associating the visible label with its trigger or forwarding the field error state.DatePickertherefore keeps its genericaria-label(for example, "Pick a date") and never exposesaria-invalid, unlike the datetime branch immediately below. Add the equivalent label/invalid plumbing to the date picker and pass it here.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
packages/apollo-wind/src/components/forms/field-renderer.tsx:518
- Checkbox fields still do not pass
requiredtoFormFieldLabel. A schema withvalidation.required: truewill reject an unchecked value, but its label has no required indicator, unlike the other built-in field types; please forward the computed required state here.
<FormFieldLabel
htmlFor={field.name}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
className="font-normal"
packages/apollo-wind/src/components/forms/metadata-form.tsx:130
- When a host already wraps
MetadataFormin a configuredTooltipProvider(for example withdelayDuration={200}), this inner default provider becomes the nearest Radix context and silently overrides the host's delay/skip-delay settings for every field tooltip. Checking whether the schema has tooltips avoids an unconditional provider, but not this nested-provider case; reuse/mark an ancestor provider or expose the provider configuration instead.
const hasFieldTooltip = useMemo(() => {
const sections = [
...(stableSchema.sections ?? []),
...(stableSchema.steps ?? []).flatMap((step) => step.sections),
];
packages/apollo-wind/src/components/forms/metadata-form.tsx:968
- The new
container="div"submit path is not exercised: the added tests cover the absence of a form and Enter suppression, but none verifies that clicking the default/custom submit action runs RHF validation and invokesonSubmitor plugin submit hooks. Add a valid and invalid submission test for this public branch.
type={action.type === 'submit' && !onSubmit ? 'submit' : 'button'}
onClick={action.type === 'submit' && onSubmit ? onSubmit : undefined}
packages/apollo-wind/src/components/forms/schema-serializer.ts:249
- These new string metadata checks use truthiness even though the renderer uses
??, so an explicitly configured emptyemptyMessageorsearchPlaceholderis meaningful. Serialization drops that value and a JSON round-trip silently restores the component default instead.
if ('emptyMessage' in field && field.emptyMessage) result.emptyMessage = field.emptyMessage;
if ('searchPlaceholder' in field && field.searchPlaceholder) {
result.searchPlaceholder = field.searchPlaceholder;
}
packages/apollo-wind/src/components/forms/string-list-field.tsx:17
token in valuesalso matches inheritedObject.prototypekeys, so an unknown placeholder such as{{toString}}is replaced with the function source instead of being left untouched as this helper promises. Use an own-property check (Object.hasOwn/hasOwnProperty) before reading the value.
token in values ? String(values[token]) : match
packages/apollo-wind/src/components/forms/validation-converter.ts:73
- The shared emptiness check still only runs for strings and arrays.
date/datetimeand numeric fields use Zod coercers, so a static required field can acceptnull(new Date(null)becomes the epoch andNumber(null)becomes0) while the conditional-requiredsuperRefinerejects that same value viaisEmptyFieldValue. Guard null/empty inputs before coercion, and cover required date/number values from JSON hosts.
if (
isArrayType(fieldType, customValueType) &&
(config.minItems == null || config.minItems < 1)
) {
schema = (schema as z.ZodArray<z.ZodTypeAny>).min(1, requiredMessage);
- Files reviewed: 26/26 changed files
- Comments generated: 0 new
- Review effort level: Lite
BenGSchulz
approved these changes
Sep 14, 2026
BenGSchulz
left a comment
BenGSchulz
left a comment
Contributor
There was a problem hiding this comment.
Appreciate you working through all the reviews. Thanks for helping close the pre-existing gaps too. This should make future migrations of custom form controls a lot easier.
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.
What changed?
Generic
forms/andui/improvements extracted from the guardrails work. The guardrailsdomain family itself lives in apollo-react — #1138, stacked on this PR — per the placement review.
Repaired: declared but never implemented
ValidationConfig.customwas typed, documented as a jsep expression and serialized, butthe converter never read it. Now enforced. Because the evaluator reports an expression it
cannot handle (
value.some(...),value.trim().length > 0— both parse, then throw on theunsupported
CallExpression) the same way it reports a legitimately failing one, a newRulesEngine.tryEvaluateExpressiondistinguishes "threw" from "returned falsey", so a schemaauthoring mistake is not enforced rather than pinning the field permanently invalid.
evaluateExpressiondelegates to it and keeps its false-on-error contract.FormPlugin.componentswas typed and never read, which is why hosts registeredasynchronously and missed the first paint. Now honoured from the first render.
plugin.onValueChangesat behind a mount-lifetime gate that could swallow a plugin'sfirst keystroke. It is now suppressed only while initialization's
resetwrites schema data,and a synchronous
onFormInitis no longer awaited into a microtask (which pushedsetIsInitializedoutside React'sact()scope and broke console-strict host suites).z.any(), whererequiredandminItemsare no-ops. They cannow declare a
valueType, so ordinary metadata constraints reach custom components.New in the forms layer
string-listfield type — repeated rows with Add/Remove (maxItems,maxLength, stablerow ids), zod conversion, and
formatTemplate.tooltip/tooltipAriaLabelfield metadata, rendered byFormFieldLabel, plus textareaminRows/maxLengthand multiselectemptyMessage/searchPlaceholder.container: 'form' | 'div'for embedding inside a host's own chrome: submit actionsbecome plain buttons wired to the form's own handler, and Enter is swallowed for single-line
inputs so it cannot trigger the host form's implicit submission. Suppressing the action row
stays the schema's job via
actions: [].MetadataFormPropsis exported, along withuseWatch, so cross-package custom fieldsshare one react-hook-form instance.
Accessibility and correctness
superRefinedisagreed:
z.string().min(1)accepts' '. Both now useisEmptyFieldValue.requiredwith an explicitminItems: 0applied only.min(0), so an empty list passed.aria-invalidis forwarded by every control that can render an error — input, textarea,select, multiselect, file upload, checkbox, switch, radio, and string-list rows — with
matching invalid styling on
SelectandTextarea(including the borderless future theme).htmlFor+id) across the renderer, including multiselect andfile upload.
InfoTooltippromoted intocomponents/ui, withFormFieldLabelowning the composition socall sites do not reassemble label + required indicator + tooltip.
refis no longer spread onto function-component custom fields.Behaviour change (ships as a
fix, not a major)A required string field now rejects whitespace-only input. Previously
' 'satisfiedrequired: trueon the resolver path, while the conditional-required path already rejectedit — the same
required: truemeant two different things depending on whether the fieldcarried rules.
Per @BenGSchulz and @CalinaCristian on this thread,
this ships as a
fixrather than a major: it is a bug fix in intent, and the prior behaviouris not something to preserve. Worth a release note for anyone who had leaned on it.
How has this been tested?
Full apollo-wind suite (1571),
tsc,biome lint(0 errors), andrslib buildclean; apollo-react's guardrailssuite (207) green against the rebuilt package. Both consumers — flow-workbench#3533
and Agents#6195 — run against the preview pair,
with Flow verified manually.
🤖 Generated with Claude Code