feat(apollo-wind): metadata-form controlled-host seam, string-list field, InfoTooltip - #1107
feat(apollo-wind): metadata-form controlled-host seam, string-list field, InfoTooltip#1107apetraru-uipath wants to merge 6 commits into
Conversation
apetraru-uipath
added
the
dev-packages
|
Apollo Coded App preview deployments finished with failures.
|
Dependency License Review
License distribution
Excluded packages
|
📦 Dev Packages
|
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.
Storybook visual diff⏭️ Skipped: the apollo-design preview deployment did not succeed, so no comparison ran. Logs Updated (PT): Sep 09, 2026, 03:45:53 PM |
📊 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
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
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
changed the title
|
Restructured per the review decision: this PR is now only the generic apollo-wind layer (forms/ MetadataForm enablers + string-list field + InfoTooltip promotion + aria-invalid styling), landable independently. The guardrails domain family moved to apollo-react (canvas) on lingui — stacked PR #1138, which also deletes the locale loader and the .gitignore negation flagged here. Design-doc record: §7.4, revised 2026-09-09. Consumers (flow-workbench#3533, Agents#6195) re-point at the apollo-react subpath next. |
github-actions
Bot
added
size:XL
There was a problem hiding this comment.
🟡 Changes recommended
StringListField does not propagate aria-invalid to its row inputs, which prevents the new invalid styling from applying and weakens accessibility semantics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Lite
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>
Review follow-up. StringListField rendered the FormFieldError text but never marked the rows invalid, so the aria-invalid styling this PR adds to Textarea never activated and assistive tech got no invalid signal for the inputs. Every row now carries it — the error belongs to the list as a whole. Also adds the InfoTooltip storybook entry requested in review (default, label-adjacent, rich and long content), documenting that an ancestor TooltipProvider is required and that schema-driven forms get the trigger from the field `tooltip` metadata. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@CalinaCristian both points are addressed — point 2 reshaped the PR entirely. 2. Placement / i18n — agreed, and done. The guardrails family is out of This PR is now only the generic wind layer you'd expect to keep there: the Your framing was also the deciding evidence: both hosts already consume 1. Storybook — done for everything this PR still adds. Both consumer PRs (flow-workbench#3533, Agents#6195) are already re-pointed at the apollo-react subpath and verified running in both products. |
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a broad new public API surface and non-trivial form-state behavior changes that should get final human review despite only minor actionable nits found.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Lite
…nts at Review follow-up: the label-adjacent story set htmlFor="blocked-phrases" with no such control, which is a dangling association and a misleading a11y example in the one story specifically about label placement. It now renders the Textarea the label names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Input fields in the updated FormFieldRenderer are not marked aria-invalid when an error exists, so invalid styling and accessibility semantics are inconsistent with the newly added aria-invalid patterns for other controls.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/forms/field-renderer.tsx:379
Inputsupportsaria-invalid:*error styling (andaria-invalidis important for assistive tech), but the text/emailInputisn’t marked invalid whenerrorexists, so invalid styling/semantics won’t activate for these fields.
This issue also appears on line 396 of the same file.
packages/apollo-wind/src/components/forms/field-renderer.tsx:400
- The number
Inputalso isn’t markedaria-invalidwhenerrorexists, so error styling/semantics won’t activate for numeric fields.
<Input
id={field.name}
value={formField.value as number | undefined}
onBlur={formField.onBlur}
name={formField.name}
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
Review follow-up: rather than a near-duplicate label component, FormFieldLabel now takes `tooltip`/`tooltipAriaLabel` and renders the trigger after the required indicator. That removes the reason the field-renderer helper existed (it composed RequiredIndicator by hand purely to control that ordering), so the helper is gone and the 11 call sites use FormFieldLabel with its `required` prop directly. The tooltip is now available to every FormFieldLabel consumer, not just schema-driven forms; the InfoTooltip story shows that path as the idiomatic one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
size:XXL
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate issues affect submission behavior, accessibility, serialization, synchronization, validation, and build correctness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (12)
Previously missed (3) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/forms/metadata-form.tsx:268
- This sync effect runs before the initialization effect below. When
stableSchema.initialDatais present, the laterreset(data)overwrites the externally suppliedvalues; because the prop has not changed, this effect does not run again. A controlled host can therefore lose its values immediately after mount. Apply the external values after initialization/reset, or otherwise re-run the sync when that reset completes.
This issue also appears on line 278 of the same file.
packages/apollo-wind/src/components/forms/validation-converter.ts:83
- Adding
string-listto the array base schema does not makevalidation: { required: true }reject an empty array. The required branch only adds a non-empty check for string fields, so a required string-list can submit[]despite showing the required indicator. Apply an array-specific minimum of one for required lists and cover the empty-list case.
packages/apollo-wind/src/components/ui/select.tsx:37 - The new invalid classes are followed by a
future:border-0style, so an invalid SelectTrigger has no visible error state in the future theme unless it is focused. The establishedInputpattern adds a future invalid ring; add the same ring to this future variant.
packages/apollo-wind/src/components/forms/field-renderer.tsx:386
- Number validation errors have the same gap: the error text is rendered, but the
Inputis not markedaria-invalid, so its invalid styling and assistive-technology state are absent. Forward the error state to this input as well.
<Input
id={field.name}
value={formField.value as number | undefined}
onBlur={formField.onBlur}
name={formField.name}
ref={formField.ref as React.Ref<HTMLInputElement>}
packages/apollo-wind/src/components/forms/field-renderer.tsx:598
- Date and datetime pickers are also rendered under these labels, but neither picker receives an id or an accessible name derived from
field.label; the picker components do not currently accept an id. The visible label is therefore not associated, and screen readers announce only the placeholder/value. Extend the picker props to forward an id/aria-label and pass them from the renderer.
<FormFieldLabel
required={required}
tooltip={field.tooltip}
tooltipAriaLabel={field.tooltipAriaLabel}
>
{field.label}
</FormFieldLabel>
packages/apollo-wind/src/components/forms/form-schema.ts:177
- These new metadata properties are not handled by
schema-serializer.ts(serializeFieldcurrently stops at the existingariaDescribedBy/old type-specific fields aroundschema-serializer.ts:209-243). A schema round-tripped throughserializeSchemaor shown/exported by the form designer will silently losetooltip,tooltipAriaLabel,minRows,maxLength,emptyMessage, andsearchPlaceholder, so the new behavior disappears from serialized forms.
/** Info tooltip rendered next to the field label. */
tooltip?: string;
/** Accessible name of the tooltip trigger (default 'More information'). */
tooltipAriaLabel?: string;
packages/apollo-wind/src/components/forms/form-schema.ts:281
- The new
string-listmetadata is also dropped byserializeField(it only handles the old field properties). Form Designer output or any schema round-trip therefore losesmaxItems,maxLength,minRows,addItemLabel, andremoveItemAriaLabel, so the rendered list falls back to generic defaults. Add these fields to the serializer and cover the round-trip.
export interface StringListFieldMetadata extends BaseFieldMetadata {
type: 'string-list';
/** Cap on how many rows can be added; the Add button hides at the cap. */
maxItems?: number;
/** Per-row character cap (DOM maxLength on each row's textarea). */
packages/apollo-wind/src/components/forms/metadata-form.tsx:260
deepEqualcompares enumerable object keys, so any twoDateinstances compare equal because they have no enumerable keys. Consequently, a controlleddateordatetimevalue change can skipsetValueand leave the form's field value stale. Use an equality check that handlesDatevalues (and other non-plain objects) before deciding to skip the sync.
if (!deepEqual(get(current, name), v)) {
packages/apollo-wind/src/components/forms/metadata-form.tsx:280
- The
appliedExternalErrorsRefguard means an external error is only re-applied when its message changes. If RHF runs the resolver after a submit or edit, it can replace atype: 'external'error with the resolver result (or clear it); while theerrorsprop remains unchanged, this effect then skipssetError, so the advertised prop-owned error disappears. Reconcile external errors after validation or merge them into the rendered error state instead of relying only on the previous message.
if (appliedExternalErrorsRef.current[name] !== message) {
form.setError(name, { type: 'external', message });
}
packages/apollo-wind/src/components/forms/metadata-form.tsx:240
- The callback is now invoked before the
isInitializedguard, so programmaticsetValuecalls made byonFormInitare reported as user edits. The built-inautoSavePluginrestores drafts withcontext.form.setValueduring initialization, which will now fireonValuesChangeand can make a controlled host echo initialization as a user change. Suppress callbacks during plugin initialization while retaining the intended first user edit.
if (!name || syncingValuesRef.current) return;
// Deliberately not behind the isInitialized gate: controlled hosts need the very first
// user edit, which can land before the async init effect finishes.
onValuesChangeRef.current?.(value as Record<string, unknown>, name);
packages/apollo-wind/src/components/forms/string-list-field.tsx:57
- When a controlled host removes a row out of band, truncating
prevby length keeps the IDs for the first positions rather than the surviving rows. For example,[a, b] -> [b]givesbthe oldakey, so React reuses the wrong DOM node and can lose the focused row, defeating the stable-row-id guarantee. Reconcile IDs with the actual removal/reorder (or keep IDs alongside the controlled items) instead of slicing by 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/ui/info-tooltip.stories.tsx:23
- This newly added Storybook documentation uses a spaced em dash. Storybook copy in this repository must not use
—; split this into two sentences (or use a colon) so the story follows the documentation convention.
Schema-driven forms get it for free — set \`tooltip\` on any field metadata and
- Files reviewed: 17/17 changed files
- Comments generated: 8
- Review effort level: Lite
Review follow-up, eight findings: - container='div' rendered schema submit actions as type="submit" with no owning <form>, so a click would submit whatever ancestor form the host embedded the fields in — the exact hazard div mode exists to avoid. Submit actions are now plain buttons wired to the form's own submit handler. - The tooltip trigger is a <button>, which <label>'s content model forbids as a descendant; it now renders beside the label in an inline wrapper, keeping the ref, htmlFor and styling on the label element. - text/email and number inputs never received aria-invalid, unlike the textarea/select paths, so their invalid styling and ARIA state never applied. - multiselect and file-upload had no label association: both now get the field id with htmlFor on the label (file upload also takes the field label as its dropzone name). - string-list never forwarded the controller's onBlur, so blur-mode validation and touched state never fired for it. - Textarea's new aria-invalid border is invisible in the future theme, which renders it borderless; it now carries the same invalid ring as Input. - The InfoTooltip story used the React UMD namespace without importing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved findings affect controlled form behavior, accessibility, serialization, validation, and error styling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (8)
Previously missed (3) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/forms/metadata-form.tsx:258
- This sync effect runs before the one-time initialization effect. When
schema.initialDatais present,initializeFormlater callsreset(data), overwriting the controlledvaluesthat were applied here; because the prop did not change, this effect will not run again. Apply controlled values after initialization (or merge them into the initialization reset) sovaluesremains the source of truth.
packages/apollo-wind/src/components/forms/metadata-form.tsx:260 - The controlled sync uses
deepEqualfor every field, but the shared helper compares only enumerable keys and treats any twoDateinstances as equal. A controlleddate/datetimevalue therefore stops updating when the host supplies a different date after the first sync. Use a comparison that handles non-plain values (or compare date timestamps) before skippingsetValue.
packages/apollo-wind/src/components/forms/metadata-form.tsx:410 - Replacing the form with a
<div>does not prevent implicit submission from an ancestor<form>: pressing Enter in a text input inside this branch can still submit the host form. That contradicts the documentedcontainer="div"guarantee of safe embedding and no implicit Enter-to-submit; handle this at the host boundary without breaking Enter in multiline controls.
packages/apollo-wind/src/components/forms/form-schema.ts:177
- The new
BaseFieldMetadata.tooltipis not rendered for checkbox fields: that renderer still callsFormFieldLabelwithouttooltiportooltipAriaLabel, so a schema can declare the new metadata but no info trigger appears for this field type. Forward both properties in the checkbox branch as done for the other built-in fields.
/** Info tooltip rendered next to the field label. */
tooltip?: string;
/** Accessible name of the tooltip trigger (default 'More information'). */
tooltipAriaLabel?: string;
packages/apollo-wind/src/components/forms/form-schema.ts:177
- The new metadata is not handled by
schema-serializer.ts'sserializeField: tooltip fields, textarea sizing/length, multiselect placeholders/messages, and all string-list options are silently dropped when a schema is serialized and loaded again. Add these properties to the serializer and cover the round trip so the new field configuration survives JSON persistence.
/** Info tooltip rendered next to the field label. */
tooltip?: string;
/** Accessible name of the tooltip trigger (default 'More information'). */
tooltipAriaLabel?: string;
packages/apollo-wind/src/components/forms/metadata-form.tsx:283
- Manual and resolver errors share RHF's single error map here. With
mode: 'onChange'oronBlur, a subsequent resolver run can remove thistype: 'external'error while theerrorsprop still contains it; conversely, clearing it below can also remove a resolver error that was overwritten. Keep host errors in an overlay/reapply them after resolver updates, or require validation to be disabled whenevererrorsis supplied.
form.setError(name, { type: 'external', message });
}
}
for (const name of Object.keys(appliedExternalErrorsRef.current)) {
if (!next[name] && form.getFieldState(name).error?.type === 'external') {
packages/apollo-wind/src/components/forms/string-list-field.tsx:114
StringListFieldignores the inheritedariaLabelmetadata, so a schema-supplied custom accessible name is silently discarded for every row. Use the metadata override as the base of each row's accessible name, falling back tofield.label.
aria-label={`${field.label} ${index + 1}`}
packages/apollo-wind/src/components/forms/validation-converter.ts:83
- The new
string-listbranches are not directly covered invalidation-converter.test.ts; the added MetadataForm string-list tests all usedisableValidation, so the base array schema andminItems/maxItemsconversion can regress without detection. Add converter cases forstring-list, including its array constraints.
case 'multiselect':
case 'string-list':
return z.array(z.string());
- Files reviewed: 17/17 changed files
- Comments generated: 6
- Review effort level: Lite
| <FormFieldLabel | ||
| required={required} | ||
| tooltip={field.tooltip} | ||
| tooltipAriaLabel={field.tooltipAriaLabel} | ||
| > | ||
| {field.label} | ||
| </FormFieldLabel> |
| <FileUpload | ||
| id={field.name} | ||
| ariaLabel={field.ariaLabel ?? field.label} | ||
| accept={field.accept} | ||
| multiple={field.multiple} |
| schema={stableSchema} | ||
| context={context} | ||
| onReset={handleReset} | ||
| onSubmit={container === 'div' ? handleFormSubmit : undefined} |
| case 'string-list': | ||
| return z.array(z.string()); |
| data-slot="select-trigger" | ||
| className={cn( | ||
| 'flex h-9 w-full cursor-pointer items-center justify-between rounded-md border border-input bg-transparent px-3 py-1 text-base transition-colors data-[placeholder]:text-muted-foreground focus:outline-none focus:ring-2 focus:ring-ring focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring disabled:cursor-not-allowed disabled:opacity-50 md:text-sm [&>span]:line-clamp-1 future:h-10 future:rounded-xl future:border-0 future:bg-surface-overlay future:hover:bg-surface-hover future:px-4 future:gap-4 future:font-normal future:text-muted-foreground', | ||
| 'flex h-9 w-full cursor-pointer items-center justify-between rounded-md border border-input bg-transparent px-3 py-1 text-base transition-colors data-[placeholder]:text-muted-foreground focus:outline-none focus:ring-2 focus:ring-ring focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring disabled:cursor-not-allowed disabled:opacity-50 aria-invalid:border-error aria-invalid:focus-visible:ring-error md:text-sm [&>span]:line-clamp-1 future:h-10 future:rounded-xl future:border-0 future:bg-surface-overlay future:hover:bg-surface-hover future:px-4 future:gap-4 future:font-normal future:text-muted-foreground', |
|
|
||
| **In forms, reach for \`FormFieldLabel\` instead of composing this by hand**: it takes | ||
| \`tooltip\` / \`tooltipAriaLabel\` and places the trigger after the required indicator for you. | ||
| Schema-driven forms get it for free — set \`tooltip\` on any field metadata and |
Follow-up review: focus on the controlled-host seamThe askKeep MetadataForm's contract as it is: values live in react-hook-form, validation lives in the schema, hosts interact through WhyThe pattern already exists in production, in one of the two consumers. flow-workbench's Three declared-but-never-implemented features are why the intended path looked insufficient. Validation is mostly expressible in the metadata contract today. Five of guardrails' seven emptiness rules already work: What to changeapollo-wind: repair the three dead features (read guardrails: move the values sync and error injection into the adapter as one documented hook. Add docs: the only story for the new Gotchas worth writing down
The counterargument
Ship as-is
|
What changed?
This PR was restructured per the review decision (thread): the guardrails domain family moved out of apollo-wind into apollo-react (canvas) — see the follow-up PR (linked below) — and this PR now carries only the generic apollo-wind layer the family is built on:
forms/— MetadataForm enablers for controlled hostsMetadataForm:values(per-field sync-in viasetValue, no full RHF reset),onValuesChange(live subscription),errors(external errors merged as RHFtype: 'external'),disableValidation, a synchronouscomponentsregistry prop, andcontainer: 'form' | 'div'for embedding inside an outer form.MetadataFormPropsis now exported.string-listfield type:StringListField(repeated rows with Add/Remove,maxItems/maxLength, stable row ids) +StringListFieldMetadata+ zod conversion; genericformatTemplateutil.tooltip/tooltipAriaLabelonBaseFieldMetadata, textareaminRows/maxLength, multiselectemptyMessage/searchPlaceholder.htmlForassociation +aria-invalidwiring,FieldLabelhelper (required indicator +InfoTooltip), and the Controllerrefis no longer spread onto function-component custom fields (fixes a React warning consumers' console-strict test suites turn into failures).useWatchre-export from the forms barrel, so packages composing MetadataForm custom fields (apollo-react's guardrails) subscribe through the same react-hook-form instance.ui/InfoTooltippromoted out of the guardrails prototype intocomponents/ui(with its a11y test).SelectandTextareagetaria-invaliderror styling.Removed relative to earlier revisions
custom/guardrails/(the whole family), its root-barrel re-exports, the./components/custom*export entries, the locale catalogs + loader, and the.gitignorenegation.custom/is fully unexported again.Why the split?
Per maintainer review + the design doc §7.4 (revised 2026-09-09): the family is domain-shaped with exactly two consumers, would have made
custom/public for the first time, and introduced a wind-local i18n mechanism against the LOCALIZATION_GUIDE. It now lives canvas-adjacent in apollo-react on lingui, built on this PR's wind layer.Follow-up PR (stacked on this one): #1138 —
feat(apollo-react): guardrails component family under canvas.How has this been tested?
tscclean, biome clean,rslib buildclean (nocustom/exports in dist).🤖 Generated with Claude Code