Skip to content

feat(apollo-react): guardrails component family under canvas - #1138

Merged
apetraru-uipath merged 1 commit into
mainfrom
feat/apollo-react-guardrails-family
Sep 15, 2026
Merged

apetraru-uipath merged 1 commit into
mainfrom
feat/apollo-react-guardrails-family

Conversation

@apetraru-uipath

@apetraru-uipath apetraru-uipath commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

What changed?

The guardrails component family moves from apollo-wind/custom/guardrails to apollo-react, per the #1107 review decision and the design doc §7.4 (revised 2026-09-09): canvas-adjacent placement at src/canvas/components/Guardrails/, MUI-free, built entirely on @uipath/apollo-wind primitives + its forms/ MetadataForm engine, strings on lingui.

Stacked on #1107: the branch contains #1107's wind commits (the family depends on those forms/ enablers), and the base is main because the dev-packages preview workflow only fires for main-based PRs — so this PR publishes a self-consistent wind + react preview pair. Review only the last commit (feat(apollo-react): ...); the wind commits are #1107's and this PR rebases to just the family once #1107 merges.

Placement & API

  • src/canvas/components/Guardrails/GuardrailBuilder (whole Add/Edit screen), GuardrailFormLayout, GuardrailValidatorForm, escalation/parameter host slots, save-time companions, type surface — public API unchanged from the wind revision except the locale prop (dropped; see i18n).
  • Exported from the canvas components barrel and a narrow ./canvas/guardrails subpath (dist/canvas/components/Guardrails/index.*) — consumers with exports-map-blind test runners (Agents' jest) map one file instead of the whole ./canvas barrel.
  • All internal wind imports go through the public @uipath/apollo-wind root barrel (the NodePropertyPanel idiom). useWatch comes from wind's re-export so custom fields share MetadataForm's react-hook-form context across the package boundary.

i18n: lingui, canvas catalog

  • The wind-local mechanism (hand-written TS catalogs + loadGuardrailMessages loader + .gitignore negation) is deleted. Strings use useSafeLingui with explicit guardrails.* ids and English defaults, via useGuardrailFormLabels/useGuardrailBuilderLabels hooks (the useStageNodeLabels pattern). The labels per-string override props survive unchanged.
  • All 780 harvested translations ported into the shared canvas catalog (src/canvas/locales/*.json, 60 ids × 13 locales; ru deliberately falls back to English per key). Hosts already mounting ApI18nProvider component="canvas" (Flow) get translations with zero changes; without a provider the components render English.
  • Localized template strings that cross into plain-string APIs (dialog titles, wind's formatTemplate) are ICU messages formatted with sentinel values reifying {token}{{token}} — translators see standard ICU, the template convention survives (see TEMPLATE_TOKENS).

CSS

  • No new artifact: tailwind.canvas.css already @source-scans src/canvas/** and wind's dist, so the family's classes ship in the existing compiled sheet. Shadow-DOM hosts inject canvas/styles/tailwind.canvas.css?inline.

Package plumbing

  • deps: class-variance-authority (GuardrailChip variants); devDeps: jest-axe + @types/jest-axe, with the matcher registered in src/test/setup.ts (first a11y assertions in this package).
  • Suites run under this package's vitest (happy-dom): 204 tests across 13 files, including the jest-axe checks.

Consumers

  • flow-workbench#3533 and Agents#6195 re-point their imports to @uipath/apollo-react/canvas/guardrails behind their existing feature flags; both pin this PR's dev-packages preview plus feat(apollo-wind): string-list field, tooltip metadata, and forms repairs #1107's wind preview (a preview of this package pins released wind, so hosts must override wind resolution to the wind preview until both release).

How has this been tested?

  • 204/204 family tests green under apollo-react's vitest (happy-dom), full package suite green, tsc clean, biome clean, lingui compile clean (the 3 pre-existing canvas.json_value_panel.item_count plural errors predate this PR), pnpm build produces dist/canvas/components/Guardrails/* (dual format) and the family's classes in tailwind.canvas.css.
  • Manual verification in both hosts via the dev previews (Flow localhost + Agents alpha with a local frontend).

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Dependency License Review

  • 1937 package(s) scanned
  • ✅ No license issues found
  • ⚠️ 2 package(s) excluded (see details below)
License distribution
License Packages
MIT 1708
ISC 88
Apache-2.0 55
BSD-3-Clause 27
BSD-2-Clause 23
BlueOak-1.0.0 8
MPL-2.0 4
MIT-0 3
CC0-1.0 3
MIT OR Apache-2.0 2
(MIT OR Apache-2.0) 2
Unlicense 2
LGPL-3.0-or-later 1
Python-2.0 1
CC-BY-4.0 1
(MPL-2.0 OR Apache-2.0) 1
Unknown 1
Artistic-2.0 1
(WTFPL OR MIT) 1
(BSD-2-Clause OR MIT OR Apache-2.0) 1
CC-BY-3.0 1
0BSD 1
(MIT OR CC0-1.0) 1
MIT AND ISC 1
Excluded packages
Package Version License Reason
@img/sharp-libvips-linux-x64 1.3.2 LGPL-3.0-or-later LGPL pre-built binary, not linked
khroma 2.1.0 Unknown MIT per GitHub repo, missing license field in package.json

@apetraru-uipath apetraru-uipath added dev-packages Adds dev package publishing on pushes to this PR and removed dev-packages Adds dev package publishing on pushes to this PR labels Sep 9, 2026
@apetraru-uipath
apetraru-uipath force-pushed the feat/apollo-react-guardrails-family branch from 31a02e7 to 129e635 Compare September 9, 2026 09:25
@apetraru-uipath
apetraru-uipath changed the base branch from feat/apollo-wind-guardrail-validator-form to main September 9, 2026 09:29
@apetraru-uipath apetraru-uipath added dev-packages Adds dev package publishing on pushes to this PR and removed dev-packages Adds dev package publishing on pushes to this PR labels Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

📦 Dev Packages

🧹 Dev packages cleaned up after PR close.

Last updated: 2026-09-15 13:04:30 PT

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Apollo Coded App preview deployments are ready.

Project Status Preview Updated (PT)
apollo-design Ready Preview · Logs Sep 15, 2026, 11:20:11 AM
apollo-docs Ready Preview · Logs Sep 15, 2026, 11:20:11 AM
apollo-landing Ready Preview · Logs Sep 15, 2026, 11:20:11 AM
apollo-vertex Ready Preview · Logs Sep 15, 2026, 11:20:11 AM

@apetraru-uipath

Copy link
Copy Markdown
Contributor Author

Note on the two red Audit checks — pre-existing and repo-wide, not from this PR. pnpm audit reads live advisory data, and five advisories currently trip it: 2 critical in next and 1 moderate in baseline-browser-mapping (both via apps/apollo-docs), 1 high in sharp, and 1 high in @tiptap/core (a pre-existing apollo-react dependency, via ap-rich-text-editor).

This PR's entire lockfile diff is 9 linesclass-variance-authority, jest-axe, @types/jest-axe — and touches none of those packages. Other open PRs (#1139, #1137) fail the identical checks, and #1107 only shows green because its run predates the advisories. Deliberately not fixing it here: bumping unrelated app dependencies does not belong in this PR.

Everything else is green — Build, Typecheck, Lint, and the test suites all pass.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

📊 Coverage + size by package

Per-package coverage and bundle size on this PR. New-line coverage = of the source lines this PR adds or changes, the % hit by tests.

Package Coverage New-line coverage Packed (gzip) Unpacked vs main
@uipath/apollo-core 42.01 MB 50.12 MB ±0
@uipath/apollo-react 44.9% 84.8% (571/673) 7.65 MB 29.55 MB +93.7 KB
@uipath/apollo-ui-icons 2.85 MB 6.91 MB ±0
@uipath/apollo-wind 461.8 KB 2.92 MB +19 B
@uipath/ap-chat 85.8% 43.93 MB 56.74 MB +7.1 KB

"Coverage" is each package's own coverage.include scope (e.g. apollo-core instruments only scripts/). "Packed"/"Unpacked" come from npm pack --dry-run and only cover built packages — "—" means not measured this run (package not affected / not built). "vs main" is the packed (gzipped) delta against the last successful main build (the package-sizes artifact from the Release workflow); "—" there means no main baseline was available this run. The baseline is main's latest build, not this PR's exact merge-base, so it includes any drift since the branch diverged. Packages with no vitest config are omitted.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Storybook visual diff

⚠️ Visual changes detected: 10 changed, 26 added (of 664 compared, 628 unchanged). View report

Baseline is the deployed main Storybook, so changes merged to main after this branch was last updated can also appear here. Logs

Updated (PT): Sep 15, 2026, 11:46:28 AM

Comment thread packages/apollo-wind/src/components/forms/metadata-form.test.tsx Fixed
@apetraru-uipath
apetraru-uipath force-pushed the feat/apollo-react-guardrails-family branch from 2ff0c35 to c9d8f20 Compare September 10, 2026 12:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved moderate issues remain across validation, accessibility, controlled state synchronization, and save behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Moves the Guardrails component family into apollo-react canvas components using Apollo Wind forms, shared Lingui catalogs, and new public exports.

Changes:

  • Adds Guardrails builders, forms, selectors, fields, slots, utilities, tests, and stories.
  • Extends Apollo Wind form and UI primitives.
  • Adds canvas translations, package dependencies, exports, and accessibility test setup.
File summaries
File Summary
pnpm-lock.yaml Locks updated dependencies.
packages/apollo-wind/src/index.ts Exposes Wind primitives and form APIs.
packages/apollo-wind/src/components/ui/textarea.tsx Adds textarea invalid-state styling.
packages/apollo-wind/src/components/ui/select.tsx Adds select invalid-state styling.
packages/apollo-wind/src/components/ui/info-tooltip.tsx Provides tooltip support.
packages/apollo-wind/src/components/ui/info-tooltip.test.tsx Tests the info tooltip.
packages/apollo-wind/src/components/ui/info-tooltip.stories.tsx Documents tooltip variants.
packages/apollo-wind/src/components/ui/index.ts Updates UI exports.
packages/apollo-wind/src/components/ui/form-field.tsx Provides form field behavior and errors.
packages/apollo-wind/src/components/ui/form-field.test.tsx Tests form fields.
packages/apollo-wind/src/components/forms/validation-converter.ts Converts form validation rules.
packages/apollo-wind/src/components/forms/validation-converter.test.ts Tests validation conversion.
packages/apollo-wind/src/components/forms/string-list-field.tsx Implements string-list editing.
packages/apollo-wind/src/components/forms/schema-viewer.tsx Renders form schemas.
packages/apollo-wind/src/components/forms/schema-serializer.ts Serializes form schemas.
packages/apollo-wind/src/components/forms/schema-serializer.test.ts Tests schema serialization.
packages/apollo-wind/src/components/forms/rules-engine.test.ts Tests form rules.
packages/apollo-wind/src/components/forms/README.md Documents the forms API.
packages/apollo-wind/src/components/forms/metadata-form.stories.tsx Documents MetadataForm usage.
packages/apollo-wind/src/components/forms/index.ts Updates forms exports.
packages/apollo-wind/src/components/forms/form-schema.ts Defines form schema types.
packages/apollo-wind/src/components/forms/form-plugins.tsx Supports form plugins.
packages/apollo-wind/src/components/forms/form-examples.tsx Provides form examples.
packages/apollo-wind/src/components/forms/form-designer.tsx Provides the form designer.
packages/apollo-wind/src/components/forms/demo-mocks.ts Provides demo mocks.
packages/apollo-wind/src/components/forms/data-fetcher.ts Provides form data fetching.
packages/apollo-wind/src/components/forms/data-fetcher.test.ts Tests data fetching.
packages/apollo-wind/src/components/forms/custom-controls.stories.tsx Documents custom controls.
packages/apollo-react/src/test/setup.ts Registers accessibility matchers.
packages/apollo-react/src/i18n/index.ts Integrates localization exports.
packages/apollo-react/src/canvas/locales/zh-TW.json Adds Traditional Chinese translations.
packages/apollo-react/src/canvas/locales/zh-CN.json Adds Simplified Chinese translations.
packages/apollo-react/src/canvas/locales/tr.json Adds Turkish translations.
packages/apollo-react/src/canvas/locales/ro.json Adds Romanian translations.
packages/apollo-react/src/canvas/locales/pt.json Adds Portuguese translations.
packages/apollo-react/src/canvas/locales/pt-BR.json Adds Brazilian Portuguese translations.
packages/apollo-react/src/canvas/locales/ko.json Adds Korean translations.
packages/apollo-react/src/canvas/locales/ja.json Adds Japanese translations.
packages/apollo-react/src/canvas/locales/fr.json Adds French translations.
packages/apollo-react/src/canvas/locales/es.json Adds Spanish translations.
packages/apollo-react/src/canvas/locales/es-MX.json Adds Mexican Spanish translations.
packages/apollo-react/src/canvas/locales/en.json Adds English Guardrails messages.
packages/apollo-react/src/canvas/locales/de.json Adds German translations.
packages/apollo-react/src/canvas/components/index.ts Exports Guardrails components.
packages/apollo-react/src/canvas/components/Guardrails/utils.ts Provides Guardrails validation utilities.
packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts Bridges controlled values and errors.
packages/apollo-react/src/canvas/components/Guardrails/types.ts Defines validator form types.
packages/apollo-react/src/canvas/components/Guardrails/render-parameter-bridge.tsx Bridges parameter overrides.
packages/apollo-react/src/canvas/components/Guardrails/index.ts Defines Guardrails exports.
packages/apollo-react/src/canvas/components/Guardrails/guardrail-validator-form.tsx Implements the controlled validator form.
packages/apollo-react/src/canvas/components/Guardrails/guardrail-form-layout.tsx Provides form layout shells.
packages/apollo-react/src/canvas/components/Guardrails/guardrail-form-layout.test.tsx Tests form layouts.
packages/apollo-react/src/canvas/components/Guardrails/guardrail-form-layout.stories.tsx Documents layout variants.
packages/apollo-react/src/canvas/components/Guardrails/form-schema-builder.ts Builds MetadataForm schemas.
packages/apollo-react/src/canvas/components/Guardrails/form-schema-builder.test.ts Tests schema generation.
packages/apollo-react/src/canvas/components/Guardrails/components/parameter-label.tsx Renders parameter labels and tooltips.
packages/apollo-react/src/canvas/components/Guardrails/components/mixed-scopes-banner.tsx Displays mixed-scope guidance.
packages/apollo-react/src/canvas/components/Guardrails/components/mixed-scopes-banner.test.tsx Tests mixed-scope messaging.
packages/apollo-react/src/canvas/components/Guardrails/components/map-enum-field.tsx Implements map threshold editing.
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-status-banner.tsx Renders status notices.
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-status-banner.test.tsx Tests status banners.
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-scope-selector.tsx Implements scope and tool selection.
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-scope-selector.test.tsx Tests scope selection.
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-chip.tsx Defines Guardrails toggle chips.
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-chip.test.tsx Tests chip behavior.
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-action-section.tsx Renders action-specific controls.
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-action-section.test.tsx Tests action controls.
packages/apollo-react/src/canvas/components/Guardrails/components/field-shell.tsx Styles composite field controls.
packages/apollo-react/src/canvas/components/Guardrails/components/field-shell.test.tsx Tests field-shell styling.
packages/apollo-react/src/canvas/components/Guardrails/components/enum-list-chips-field.tsx Implements enum-list chip editing.
packages/apollo-react/src/canvas/components/Guardrails/builder-utils.ts Provides builder initialization helpers.
packages/apollo-react/src/canvas/components/Guardrails/builder-utils.test.ts Tests builder utilities.
packages/apollo-react/src/canvas/components/Guardrails/builder-types.ts Defines builder types and slots.
packages/apollo-react/package.json Adds exports and dependencies.
Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (10)

packages/apollo-react/src/canvas/components/Guardrails/README.md:169

  • This section documents MetadataForm props (values, onValuesChange, errors, disableValidation, and components) that are not part of the current exported MetadataFormProps; the adapter actually passes a plugins bridge and container="div". Consumers following this documentation cannot implement the described integration. Update the stack description to match the plugin-based API (or add and export the documented props before publishing).
`GuardrailValidatorForm` is not a form renderer of its own: internally it is
`buildGuardrailFormSchema(definitions, labels)` + the package's `MetadataForm`
(`components/forms/`: `FormSchema` → `MetadataForm` → `field-renderer`), mounted through the
controlled-host seam (`values` / `onValuesChange` / `errors` / `disableValidation` /
`container="div"` / synchronous `components`). The public contract above is the adapter

packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-scope-selector.tsx:114

  • These <Label> elements are not associated with a form control: the actual controls are buttons inside the following FieldShell, so the scope/tool heading is neither clickable nor announced as the group name. Render each set as a fieldset/legend (or a labelled group with aria-labelledby) and expose the group’s invalid state.
        <Label>
          {labels.scopesLabel}
          <RequiredIndicator />
        </Label>
        <FieldShell invalid={Boolean(errors?.scopes)}>

packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-scope-selector.tsx:103

  • Filtering selectedTools down to availableToolNames hides stale targets without removing them from selector.matchNames. If a tool is removed while another remains, the user cannot see or deselect the stale name and the builder still saves it; reconcile matchNames when the available tool list changes or render stale targets for removal.
  const targetedTools = useMemo(
    () => selectedTools.filter((name) => availableToolNames.includes(name)),
    [selectedTools, availableToolNames]
  );
  const addableTools = useMemo(
    () => availableToolNames.filter((name) => !selectedTools.includes(name)),
    [availableToolNames, selectedTools]

packages/apollo-react/src/canvas/components/Guardrails/components/mixed-scopes-banner.tsx:29

  • Tool names are host-supplied, so a tool named Agent (or another scope value) collides with the scope key in this same <ul>. React will warn about duplicate keys and can reconcile the list incorrectly; prefix keys by item category.
          {otherAppliedScopes.tools.map((tool) => (
            <li key={tool}>{tool}</li>
          ))}

packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx:364

  • The builder forwards errors.parameters but does not pass GuardrailValidatorForm's onClearError, and it exposes no parameter-change callback. Consequently, once a host parameter error is supplied, editing that parameter cannot clear it from this whole-screen API; the host has no way to observe the edit and hasHostErrors continues to block Save. Expose/forward a parameter-error clear or change callback, or define an internal clearing policy.
          <GuardrailValidatorForm
            parameterDefinitions={definition.parameters}
            parameters={formData.validatorParameters}
            onChange={(params) => updateField('validatorParameters', params)}
            errors={validatorFormErrors}
            renderParameter={renderParameter}

packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx:255

  • Empty host messages are treated as errors here because the predicate only checks whether the field or parameter map has a key. Clearing errors.name or errors.parameters[id] to '' therefore leaves Save disabled while no message is rendered; treat empty messages as absent, consistently with the validator bridge.
  const hasHostErrors = Boolean(
    hostErrors &&
      Object.values(hostErrors).some(
        (v) => v !== undefined && (typeof v !== 'object' || Object.keys(v).length > 0)
      )

packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx:245

  • The merge treats any defined host value as an override, so name: '' or parameters: { id: '' } replaces an existing internal error with an empty message. After a failed Save, clearing a host error can therefore leave the form blocked with no visible error; merge only non-empty host messages (including nested parameter entries).
      for (const [key, value] of Object.entries(hostErrors)) {
        if (value !== undefined) {
          (base as Record<string, unknown>)[key] =
            key === 'parameters'
              ? { ...base.parameters, ...(value as Record<string, string>) }
              : value;
        }

packages/apollo-react/src/canvas/components/Guardrails/guardrail-validator-form.tsx:191

  • This adapter is documented as validation-free, but the schema passed here still contains required/min/max validation and useMetadataFormBridge selects onChange; MetadataForm therefore runs its resolver and emits its own generic errors alongside host-provided errors. That breaks the controlled contract when a user clears a required field or enters an out-of-range value. Pass the intended validation-disable option through the MetadataForm seam, or omit these validation rules before mounting this form.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:78
  • When a controlled host removes a defined parameter from parameters, values simply omits that key, but this effect only writes present entries. React Hook Form therefore retains the old value and the UI can show or re-emit a parameter the host deleted; reconcile missing defined fields as well as syncing present values.
    packages/apollo-react/src/canvas/components/Guardrails/utils.ts:170
  • Required map-enum validation checks the raw map keys, but those keys can be stale relative to the keySource. syncMapEnumParameters later prunes the map to the current selection; therefore a non-empty default/stale map plus an empty source selection is reported as valid here and can be persisted as an empty required map. Validate the reconciled map, or derive the effective key set from keySource before deciding that the parameter is filled.
  • Files reviewed: 86/87 changed files
  • Comments generated: 8
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/apollo-react/src/canvas/components/Guardrails/form-schema-builder.ts Outdated
Comment thread packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx Outdated
Comment thread packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx Outdated
Comment thread packages/apollo-react/src/canvas/components/Guardrails/utils.ts
Copilot AI review requested due to automatic review settings September 10, 2026 13:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved validation and host/resolver error-handling issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (6)

packages/apollo-react/src/canvas/components/Guardrails/form-schema-builder.ts:52

  • This condition is the only path that copies min/max into MetadataForm validation. map-enum rows are numeric too, but their custom editor receives only DOM bounds and the save-time helper, so an out-of-range map row has no live resolver error. That contradicts the documented live min/max validation; add map-row validation (likely a record-valued custom type) or explicitly document map ranges as host-only.
  if (def.type === 'number') {

packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:151

  • The external-error effect only runs when the errors prop changes, but this form validates on every edit (schemaMode: 'onChange'). React Hook Form can clear a manually set type: 'external' error when the edited value passes the resolver; with an unchanged host error, this effect will not restore it. GuardrailBuilder also does not pass onClearError, so the message can disappear while hasHostErrors still keeps Save disabled. Re-apply the current external error after value/validation updates (or otherwise preserve it outside resolver updates) and add a regression test that edits a field while its host error remains unchanged.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:147
  • clearErrors clears the field's entire error entry, not only the external error that this bridge installed. When the resolver also considers the current value invalid, removing the host error therefore hides the resolver error and no validation is triggered to restore it. Revalidate the field after clearing the external error (or otherwise preserve the resolver error).
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:89
  • This controlled-value sync uses setValue without shouldValidate, so React Hook Form does not re-run the schema resolver for an external value change. If a field has a live required/range error and the host later canonicalizes or replaces its value with a valid one, the stale resolver error can remain visible until another user edit. Revalidate changed values, while preserving an already-applied external error so the documented host-error precedence is not lost.
    packages/apollo-react/src/canvas/components/Guardrails/utils.ts:139
  • The map editor intentionally retains keys that are no longer selected until save, but this check scans every stored map value. If a user sets a removed entity's threshold out of range and then deselects that entity, Save remains blocked even though syncMapEnumParameters will remove the invalid key from the persisted map. Validate the reconciled map (or restrict this scan to the current keySource selection) before reporting the error.
    packages/apollo-wind/src/components/forms/validation-converter.ts:312
  • The new customValueType plumbing is applied to arrays here, but not to applyNumberConstraints above: valueType: 'number' custom fields therefore skip min/max/integer/positive/negative entirely because the numeric helper still checks only fieldType. This contradicts the typed-custom-field contract and affects Guardrails' numeric custom override; thread the value type through the numeric helper as well and add a custom-number constraint test.
  • Files reviewed: 82/83 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread packages/apollo-react/src/canvas/components/Guardrails/i18n.ts Outdated
apetraru-uipath added a commit that referenced this pull request Sep 14, 2026
…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.
- 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>
Copilot AI review requested due to automatic review settings September 14, 2026 08:36
@apetraru-uipath
apetraru-uipath force-pushed the feat/apollo-react-guardrails-family branch from 70288ca to a011fad Compare September 14, 2026 08:36
apetraru-uipath added a commit that referenced this pull request Sep 14, 2026
…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>
@apetraru-uipath
apetraru-uipath force-pushed the feat/apollo-react-guardrails-family branch from a011fad to 6bf68ba Compare September 14, 2026 08:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved review findings remain, including form synchronization and validation behavior.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (10)

packages/apollo-react/src/canvas/components/Guardrails/README.md:91

  • GuardrailValidatorForm mounts MetadataForm, which now supplies its own TooltipProvider whenever field metadata contains a tooltip. Requiring every validator-form consumer to add an ancestor provider is therefore inaccurate; only independently rendered tooltip content needs one.
Requires an ancestor `TooltipProvider` (for the per-parameter info tooltips).

packages/apollo-react/src/canvas/components/Guardrails/form-schema-builder.ts:235

  • For enum-list fields, selected is used as the default value but these options remain definition-only. If a stored selection is no longer in options, the large-list path renders an unlabeled badge with Remove undefined and the popup has no item to remove; the chip path hides it entirely. Since save reconciliation keeps the value, it can be persisted indefinitely. Add missing selected values as synthetic options, or explicitly normalize them out, consistently in both render paths.
        options: (def.options ?? []).map((opt) => ({
          value: opt,
          label: def.optionLabels?.[opt] ?? opt,
        })),
        defaultValue: selected,

packages/apollo-react/src/canvas/components/Guardrails/i18n.ts:10

  • This comment is stale: requiredError, minError, and maxError are now part of the label interface and are passed into buildFieldValidation, so resolver validation messages are not host-owned exclusively. Update the contract comment to distinguish these localized schema messages from host-owned domain/save-time errors.
    packages/apollo-react/src/canvas/components/Guardrails/render-parameter-bridge.tsx:47
  • This bridge drops props.error, which is the MetadataForm/RHF error for the custom field, and forwards only host errors. A renderParameter override with a declared valueType (for example, a required enum) therefore never receives the form's live resolver error unless the host duplicates it in errors, contrary to the shared-validation contract. Prefer the host error when present and otherwise pass through props.error.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:148
  • When the host removes an external error while the value still violates the resolver, clearErrors clears the entire RHF error and the resolver is not run again. The invalid field then appears valid until another edit, even though this form uses live validation. Re-trigger validation after clearing the external error (or otherwise restore the resolver error).
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:88
  • The controlled values record omits parameters that are absent from the host array, but this loop only writes present entries. If a host removes a parameter after it had a value, the RHF field retains and displays that old value, violating the documented controlled contract. Track the previous field names and explicitly reset omitted fields to the definition's default/empty value, with a regression test for removal.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:90
  • Controlled values are written with setValue without requesting validation. When a host changes a value externally to an invalid required or range value, the control updates but MetadataForm's resolver state can remain stale until another user event, so the standalone form may not show the error. Revalidate external updates after initialization while preserving the current no-error-on-mount behavior.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:143
  • Because this effect only calls setError when the message changes, React Hook Form's on-change resolver can remove an external error after the user edits a field into a resolver-valid value while the host errors prop still contains the same message. The dependency on errors does not rerun for that field edit, so the documented host-owned error can disappear. Re-apply host errors after form changes (or subscribe to field state) and cover an edit-with-unchanged-errors case.
    packages/apollo-react/src/canvas/components/Guardrails/utils.ts:100
  • When the source enum-list has no selected keys, syncMapEnumParameters produces {} for its dependent map. This cleanup only drops strings and arrays, so an optional map-enum is still emitted with an empty object instead of being omitted, contrary to the helper's empty-value contract and the map's own empty predicate. Treat empty non-array records as empty here.
    packages/apollo-react/src/canvas/locales/ru.json:11
  • The PR description and Guardrails README state that Russian deliberately falls back to English per key, but these four new Guardrails entries provide Russian translations. Russian hosts will therefore receive a partial Russian catalog rather than the stated fallback behavior; either remove these entries or update the documented/accepted localization behavior.
  • Files reviewed: 83/84 changed files
  • Comments generated: 2
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 14, 2026 08:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unresolved review findings remain in accessibility, localization, validation, and controlled-form synchronization.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (10)

packages/apollo-react/src/canvas/components/Guardrails/components/parameter-label.tsx:25

  • When a parameter has a tooltip, this puts the real <button> trigger inside the <label> returned below. That is the same invalid labelable-descendant structure that FormFieldLabel explicitly avoids, and clicking the info button can ambiguously activate the associated control. Use FormFieldLabel for the normal label path (passing required, tooltip, and tooltipAriaLabel) so the trigger is rendered beside, not inside, the label.
      {paramDef.tooltip && (
        <InfoTooltip content={paramDef.tooltip} aria-label={labels.moreInformation} />
      )}

packages/apollo-react/src/canvas/components/Guardrails/form-schema-builder.ts:232

  • Definitions that take this multiselect branch only localize the trigger placeholder. field-renderer and MultiSelect then fall back to English for the search placeholder, empty-results text, clear-all action, and remove-item labels, so a localized guardrail still exposes English chrome when a large enum list is opened. Add these strings to the guardrail label/schema surface (including the relevant MultiSelect chrome) or provide a localized guardrail wrapper.
      return {
        ...base,
        type: 'multiselect',
        placeholder: labels.enumListPlaceholder,
        options: (def.options ?? []).map((opt) => ({
          value: opt,

packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx:184

  • The builder's save gate only checks requiredness and numeric ranges here. maxItems and maxLength are passed to the renderer, but an existing or externally controlled text-list/text value can exceed those limits and is still emitted by onSave because the MetadataForm resolver is not part of isValid. Add save-time validation for these definition constraints (or otherwise include the resolver result in the builder gate) before allowing either save action.
    const trimmed = syncMapEnumParameters(formData.validatorParameters, definition.parameters).map(
      (param) =>
        param.$parameterType === 'text-list'
          ? { ...param, value: param.value.filter((entry) => entry.trim().length > 0) }
          : param
    );
    const validatorParameters = dropEmptyOptionalParameters(trimmed, definition.parameters);

packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx:221

  • These predicates run on the raw map before the save-time syncMapEnumParameters reconciliation. Because the map editor intentionally retains deselected keys, a required map can appear non-empty and save as {} after reconciliation; an invalid stale row can also block a save even though it will be pruned. Validate the reconciled parameter array here (or make both helpers reconcile before inspecting map values).
    const parameterErrors: Record<string, string> = {};
    for (const id of getRequiredEmptyParameterIds(
      definition.parameters,
      formData.validatorParameters
    )) {

packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx:418

  • MixedScopesBanner always renders the “Use Save as new” instruction whenever otherAppliedScopes is supplied, but the Save-as-new action is optional (onSaveAsNew). Hosts that only provide the banner therefore show an instruction for a button that is absent. Make the hint conditional on onSaveAsNew (while retaining the scope list), or require the callback whenever this banner is shown.
      {/* Mixed scopes info banner */}
      <MixedScopesBanner otherAppliedScopes={otherAppliedScopes} labels={labels} />

packages/apollo-react/src/canvas/components/Guardrails/guardrail-validator-form.stories.tsx:27

  • This Storybook description still calls the component “validation-free” and says the host owns all validation, but the implementation now declares required/range rules and renders live MetadataForm resolver errors. That description tells consumers to provide errors for behavior the component already performs; update it to distinguish schema-owned live validation from host-owned domain/save validation.
    packages/apollo-react/src/canvas/components/Guardrails/i18n.ts:10
  • This comment says the required-error message is deliberately absent and all validation messages are host-owned, but requiredError is now part of the public form-label type and is used to localize the resolver's live required errors. Update the documentation so it does not contradict the current labels contract.
    packages/apollo-react/src/canvas/components/Guardrails/render-parameter-bridge.tsx:47
  • RenderParameterBridge passes only ctx.errors into the host override, so resolver failures from a declared valueType are discarded. For example, a required custom enum with no errors entry gets an RHF error, but the override cannot render it, contradicting the live-validation contract. Prefer the host message when present and fall back to the field error (ctx.errors?.[props.name] ?? props.error).
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:90
  • The controlled sync only writes entries present in next; it never clears a React Hook Form field when the host removes that parameter. For example, changing parameters from [{ id: 'p', value: 'x' }] to [] leaves x rendered in the supposedly controlled editor, and later edits can still be based on that stale value. Track the form field names/previous values and reset fields missing from the host snapshot to their defaults (or an empty value).
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:89
  • External controlled updates use setValue without shouldValidate. After a user enters an invalid range, if the host then sends a corrected value, React Hook Form's onChange resolver error remains in formState.errors, so the editor continues showing the old error even though its value is valid. Request validation when syncing a changed value while keeping the existing syncingRef suppression for emissions.
  • Files reviewed: 83/84 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

apetraru-uipath added a commit that referenced this pull request Sep 14, 2026
…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>
Copilot AI review requested due to automatic review settings September 14, 2026 09:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (6)

packages/apollo-react/src/canvas/components/Guardrails/README.md:87

  • The usage snippet labels errors as entirely host-owned validation, while the contract below correctly documents shared validation and the form's live resolver errors. Update this inline explanation to identify errors as host-provided domain/save-time messages, otherwise the snippet contradicts the API description immediately below.
<GuardrailValidatorForm
  parameterDefinitions={definition.parameters}
  parameters={parameters}
  onChange={setParameters}
  errors={errors}                 // Record<paramId, message> — host-owned validation
  onClearError={clearParamError}

packages/apollo-react/src/canvas/components/Guardrails/components/map-enum-field.tsx:58

  • If the source enum-list has no selected keys, this early return also discards error. A required map-enum parameter (or a host error for it) can therefore prevent Save without displaying any map-enum validation message; the builder test currently checks only that save is blocked. Keep the empty-state behavior, but render the field error before returning when one is present.
  if (keys.length === 0) return null;

packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx:108

  • The builder exposes only Partial<GuardrailBuilderLabels>, but it does not expose or forward the GuardrailValidatorFormLabels accepted by the nested validator form. Consequently a consumer using GuardrailBuilder cannot override the live resolver messages (requiredError, minError, maxError) even though the public docs promise that labels overrides any message and the standalone validator supports those overrides. Add a validator-labels override to the builder API and pass it through, or narrow the documented guarantee.
  labels?: Partial<GuardrailBuilderLabels>;

packages/apollo-react/src/canvas/components/Guardrails/guardrail-validator-form.stories.tsx:27

  • The story documentation says this component is "validation-free" and that the host owns validation, but the implementation now declares required/min/max in the MetadataForm schema and runs live resolver validation. This gives consumers the wrong contract; please describe the validation as shared and reserve host validation for domain/save-time errors.
    packages/apollo-react/src/canvas/components/Guardrails/guardrail-validator-form.stories.tsx:220
  • This story's comment also describes validation as host-owned, although the example only supplies the host's save-time required-field errors and the form still performs live schema validation. Please call out that this is the host-owned error/save-time portion to avoid contradicting the component contract.
    packages/apollo-react/src/canvas/components/Guardrails/i18n.ts:10
  • This comment still says required-error messages are host-owned and absent, but GuardrailValidatorFormLabels defines requiredError and useGuardrailFormLabels resolves it from the Lingui catalog; the schema uses it for live resolver errors. Please update the comment to distinguish localized resolver messages from host-owned domain/save-time errors.
  • Files reviewed: 83/84 changed files
  • Comments generated: 1
  • Review effort level: Lite

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved validation, stale map normalization, error propagation, and locale-contract issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (8)

packages/apollo-react/src/canvas/components/Guardrails/components/escalate-action-fields.tsx:257

  • When no renderAppPicker slot is supplied, this fallback renders the required label and unavailable notice but never renders errors?.actionApp. The builder still blocks Save for a missing app, leaving the user without the validation message; render the field error in this fallback as well (using the same FormFieldError used by the recipient field and the host slot context).
          <>
            <Label>
              {labels.actionAppLabel}
              <RequiredIndicator />
            </Label>
            <Alert variant="info">
              <Info />
              <AlertDescription>{labels.appPickerUnavailable}</AlertDescription>
            </Alert>

packages/apollo-react/src/canvas/components/Guardrails/guardrail-validator-form.stories.tsx:27

  • This description says the component is "validation-free" and directs hosts to compute required-field validation, but the implementation builds a resolver-backed MetadataForm that performs live required/range validation (and documents that contract in guardrail-validator-form.tsx). This misleads consumers about which errors are supplied by the form versus the host; describe schema validation as internal and reserve errors for host/domain validation.
    packages/apollo-react/src/canvas/components/Guardrails/render-parameter-bridge.tsx:49
  • The custom field renderer passes resolver failures as props.error, but this bridge discards that value and forwards only ctx.errors to renderParameter. Consequently an override with a declared valueType can fail MetadataForm's required/type validation while its slot receives no error and renders no feedback; preserve the host error precedence while falling back to props.error.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:148
  • When a host error is removed, clearErrors removes the field's current error without rerunning the resolver. If the value is still invalid according to the MetadataForm schema, the resolver error that was replaced by the external error is lost until another validation-triggering interaction, so consumers can see no error for an invalid value; trigger validation for the field after clearing the external error.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:143
  • The bridge applies a host error only when the errors prop changes. React Hook Form's onChange resolver can clear a setError entry when the edited value passes the schema, so an unchanged host error disappears after editing even though this contract says host errors remain until the prop drops them. Reapply the current external error after field changes (or otherwise preserve it independently of resolver validation).
    packages/apollo-react/src/canvas/components/Guardrails/utils.ts:100
  • syncMapEnumParameters can produce {} for an optional map-enum when its source selection is empty, but this predicate only recognizes empty strings and arrays. The builder therefore persists an empty optional map instead of dropping it, contrary to this helper's purpose and the runtime constraint described above; include empty map-enum records in the predicate.
    packages/apollo-react/src/canvas/components/Guardrails/utils.ts:130
  • getOutOfRangeParameterIds reads the raw map-enum object before syncMapEnumParameters removes keys that are no longer selected by keySource. A user can deselect a row whose stale threshold is out of range; the row disappears, but this hidden value still blocks Save even though the emitted value will be pruned. Normalize the parameters against the current key selection before checking map entries.
    packages/apollo-react/src/canvas/locales/ru.json:11
  • This catalog adds Russian translations for only these four guardrail messages, while the PR description and the Guardrails README state that ru deliberately falls back to English per key. With the Russian provider active, these messages therefore behave differently from the rest of the family. Remove these entries (or update the documented fallback policy) so the shipped behavior matches the stated locale contract.
  • Files reviewed: 57/58 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread packages/apollo-react/src/canvas/components/Guardrails/utils.ts

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved build, form-submission, and validation-error handling issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (1)

packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:148

  • When a host error is removed, clearErrors(name) clears the entire RHF error entry. If the field also has a resolver failure (for example, a required empty value), that resolver error is not restored because clearErrors does not re-run validation, so the field appears valid until the next edit. Re-trigger validation after clearing the external error (or otherwise preserve the resolver error).
  • Files reviewed: 57/58 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment thread packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx Outdated
Comment thread packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Four unresolved moderate findings remain, including validation-state synchronization issues and a Storybook documentation violation.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (4)

packages/apollo-react/src/canvas/components/Guardrails/guardrail-validator-form.stories.tsx:29

  • This Storybook description uses a spaced em dash, but the repository's Storybook documentation rule prohibits in .stories.tsx copy (CLAUDE.md). Replace it with a period or colon so the story complies with the documented content convention.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:148
  • When a host error is removed, the earlier setError(..., { type: 'external' }) may have overwritten a resolver error for the same field. clearErrors(name) then removes both errors, so a required/range-invalid value can become visually valid after the user edits it; re-run the resolver (or otherwise preserve its error) after clearing the external one.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:115
  • When a field is edited, React Hook Form re-runs the resolver and can replace this type: 'external' error. Because this effect only runs when errors changes, a standalone consumer that keeps the same errors prop (or an edit that does not clear the host verdict) loses the host error even though the prop remains the source of truth. Re-apply the current host error after value validation, or otherwise preserve external errors on edits, and add a regression test for an edit with an unchanged errors prop.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:89
  • When the host changes a controlled value externally (for example, restoring an out-of-range threshold), this programmatic update does not request validation. GuardrailValidatorForm forces the embedded form to onChange mode, but React Hook Form does not run the resolver for setValue by default, so the displayed value can retain stale or missing resolver errors until the user edits it. Pass validation options here and cover an invalid external-value update in the form test.
  • Files reviewed: 57/58 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (4)

packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx:199

  • The ancestor-form guard treats every input except type="button" as a text field. That also cancels Enter on checkbox, radio, submit/reset, file, and other host-slot controls, preventing their normal keyboard activation. Restrict this to the text-like input types that can implicitly submit instead of swallowing Enter for every HTMLInputElement.
    const swallowEnter = (event: KeyboardEvent) => {
      if (event.key !== 'Enter' || event.defaultPrevented) return;
      const target = event.target as HTMLElement;
      if (target instanceof HTMLInputElement && target.type !== 'button') {
        event.preventDefault();
      }

packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:148

  • When a host error is withdrawn while the field also has a resolver error, clearErrors(name) clears both errors because React Hook Form does not retain the resolver error underneath setError. The local required/range message then disappears until the next edit, even though the value is still invalid. Re-run validation after removing the external error so only the host error is removed.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:88
  • This only syncs keys present in next; when the controlled host removes a defined parameter (for example, resetting parameters to []), the old React Hook Form value remains rendered and can be emitted again on the next edit. The controlled contract says external parameter changes are authoritative, so missing registered fields also need to be reset to their schema/default value.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:89
  • setValue does not run the resolver unless validation is requested, so a controlled host echo can leave a stale error attached to the field. For example, clearing a number emits NaN, the bridge coerces the host value to 0, but the resolver's NaN error remains after the echo; external value updates also never get live validation. Request validation when syncing the value so the displayed error matches the controlled value.
  • Files reviewed: 57/58 changed files
  • Comments generated: 1
  • Review effort level: Lite

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Six moderate findings and one nit remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (5)

packages/apollo-react/src/canvas/components/Guardrails/components/escalate-action-fields.tsx:163

  • The fixed escalate-recipient-type ID is duplicated whenever more than one escalation editor is mounted, causing the label association to resolve unpredictably across builders. Generate this ID with an instance-specific useId prefix, as for the other builder controls.
        <Label htmlFor="escalate-recipient-type">{labels.assignToLabel}</Label>
        <Select value={String(displayedRecipientType)} onValueChange={handleRecipientTypeChange}>
          <SelectTrigger
            id="escalate-recipient-type"
            aria-label={`${labels.assignToLabel}: ${recipientTypeLabels[displayedRecipientType] ?? labels.recipientFallbackLabel}`}

packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-action-section.tsx:68

  • This fixed action-type ID (and the severity-level/block-reason IDs below) is shared by every GuardrailActionSection instance. Multiple guardrail editors therefore produce duplicate IDs, so htmlFor/accessible-name lookups can target the wrong control. Use an instance-specific useId prefix for each label/control pair.
        <SelectTrigger id="action-type">

packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:89

  • setValue does not run React Hook Form's resolver unless shouldValidate is set. When the controlled host changes a field from valid to required/range-invalid (or back), this bridge updates the displayed value but leaves the previous validation state untouched; the documented live validation only runs for user edits. Pass { shouldValidate: true } here while retaining the sync guard so external updates are revalidated.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:90
  • This loop only writes keys present in next, so a controlled parameters update that omits a previously present definition never clears that field from React Hook Form. The old value remains visible and can be emitted again on a later edit even though the host no longer owns it. Track the previous/schema field names and clear or reset fields absent from next after the initial schema defaults are established.
    packages/apollo-react/src/canvas/locales/tr.json:197
  • The Turkish user-search placeholder ends with two periods (ara..), unlike the ellipsis used by the corresponding placeholder and the other catalog entries. Use a three-dot ellipsis so the placeholder is rendered consistently.
  • Files reviewed: 57/58 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved moderate findings remain in Guardrails identifiers, state synchronization, validation, and empty-value handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (7)

packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-action-section.tsx:136

  • The block-reason field also uses a document-global id. When two builders render a block action, duplicate block-reason ids make the labels ambiguous and can associate a label with the other builder's input; namespace both sides with this instance's uid.
              id="block-reason"

packages/apollo-react/src/canvas/components/Guardrails/components/mixed-scopes-banner.tsx:28

  • The two loops share the raw item as the React key, so a scope label and a tool name with the same text produce duplicate keys. This can make React reuse the wrong <li> when mixed-scope data changes and emits duplicate-key warnings. Prefix keys by collection (scope- versus tool-).
            <li key={s}>{s}</li>
          ))}
          {otherAppliedScopes.tools.map((tool) => (
            <li key={tool}>{tool}</li>

packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx:332

  • The nested metadata-form bridge ignores falsy host messages (if (!message) continue), but this gate treats errors={{ name: '' }} as an active error and treats a parameter map containing only empty messages as active too. A host that clears an error with an empty string will therefore still be unable to save, contrary to the documented error contract. Count only truthy messages here, matching the bridge.
        if (key === 'parameters') return Object.keys(liveHostParameterErrors).length > 0;
        return typeof v !== 'object' || Object.keys(v).length > 0;

packages/apollo-react/src/canvas/components/Guardrails/guardrail-validator-form.tsx:170

  • When a controlled host removes a parameter from parameters (for example, replacing a populated array with []), this map omits the field entirely. useMetadataFormBridge only iterates the incoming entries when syncing, so React Hook Form keeps the old value rendered and a later edit can re-emit that stale parameter. The controlled adapter needs to provide/clear every defined field, including ids absent from the host array.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:89
  • The controlled-value sync calls RHF setValue without requesting validation. If a host changes a required or ranged value externally, the input updates but MetadataForm's live resolver error remains stale until the user edits that field; clearing a host error can therefore leave invalid data unmarked. Pass shouldValidate: true when applying a changed value.
    packages/apollo-react/src/canvas/components/Guardrails/utils.ts:100
  • An optional map-enum with no selected keys is reconciled to {} by syncMapEnumParameters, but this cleanup only drops empty strings and arrays. The final guardrailResult therefore still sends an empty optional map instead of omitting it, even though this helper is intended to remove values runtimes reject when empty. Include empty map-enum objects in the empty-value predicate.
    packages/apollo-react/src/canvas/locales/ru.json:11
  • The PR description and Guardrails README state that Russian deliberately falls back to English per key, but these entries add Russian translations for four messages. With this catalog loaded, those four render in Russian while the remaining Guardrails strings fall back to English, so either remove these entries to preserve the stated behavior or revise that requirement.
  • Files reviewed: 57/58 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread packages/apollo-react/src/canvas/components/Guardrails/components/field-shell.tsx Outdated
Comment thread packages/apollo-react/src/canvas/components/Guardrails/utils.ts Outdated
Comment thread packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.test.tsx Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved validation and form-state issues block safe approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (4)

packages/apollo-react/src/canvas/components/Guardrails/render-parameter-bridge.tsx:51

  • The bridge treats empty host messages as cleared (useMetadataFormBridge skips them), but this override path uses nullish coalescing, so errors={{ [id]: '' }} still replaces the resolver error with an empty string. A custom parameter can therefore lose its live required/range message while the host error map is being cleared; use the host message only when it is non-empty.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:90
  • When a controlled host removes a previously present parameter (for example, after dropping an optional empty value), its id is absent from next, so this loop never clears the corresponding React Hook Form field. The old value remains visible and can be emitted again on the next edit even though parameters no longer contains it. Track previously synced ids and explicitly clear absent fields (without disturbing the initial schema defaults) before applying new values.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:165
  • When a host error is removed, clearErrors(name) removes the entire RHF error entry, including any resolver error that setError(..., { type: 'external' }) previously replaced. For example, clearing a host error while a required field is still empty makes the standalone validator form appear valid until the next edit. Re-run validation for this field after clearing the external error so the schema error is restored.
    packages/apollo-react/src/canvas/locales/tr.json:202
  • This localized placeholder has two periods rather than the ellipsis used by the other search placeholders, so Turkish users see a punctuation typo in the input hint. Use Bir kullanıcı ara....
  • Files reviewed: 57/58 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment thread packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx Outdated
Comment thread packages/apollo-react/src/canvas/components/Guardrails/utils.ts Outdated
Comment thread packages/apollo-react/src/canvas/components/Guardrails/utils.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unresolved moderate issues remain in form synchronization, enum handling, validation, accessibility, and duplicate field IDs.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (14)

packages/apollo-react/src/canvas/components/Guardrails/README.md:145

  • This public integration documentation says edit-mode values pass through verbatim, but the builder normalizes persisted validator parameters before placing them in form state. That makes the documented edit contract inaccurate; describe the normalization here as well.
- `seedGuardrailParameters` builds the initial value array from definitions (editing passes
  the stored values through verbatim), coercing `null` defaults to the union's value types.

packages/apollo-react/src/canvas/components/Guardrails/builder-utils.ts:97

  • This initialization contract still says edit values are copied verbatim, but the implementation below calls normalizeGuardrailParameters for persisted validator parameters. Update the description so it reflects the actual normalization and does not promise verbatim preservation.
 * Initial form state: an existing guardrail is copied verbatim (edit); otherwise values are
 * seeded from the definition — parameters via `seedGuardrailParameters`, the scope coerced to
 * the definition's first allowed scope when `Agent` is not allowed, a log/Info default
 * action, and evaluations enabled.

packages/apollo-react/src/canvas/components/Guardrails/components/enum-list-chips-field.tsx:34

  • The chip editor renders an unattached ParameterLabel and places the option buttons in an unlabelled div. Unlike the standard multiselect path, assistive technology gets names such as “Email button” without the parameter context (“Entities”), so the field label is not conveyed. Give the chip container a labelled group (and carry its invalid state) instead of leaving the label orphaned.
    <FormField data-slot="guardrail-enum-list-field">
      <ParameterLabel paramDef={paramDef} labels={labels} />
      <FieldShell invalid={Boolean(error)}>
        <div className="flex flex-wrap gap-1.5">
          {(paramDef.options ?? []).map((option) => (

packages/apollo-react/src/canvas/components/Guardrails/components/escalate-action-fields.tsx:153

  • The save gate treats an app as selected when action.app.id is non-empty, but the slot receives null whenever only name is empty. A persisted app with a valid id and missing display metadata therefore passes validation while the host picker shows it as unselected, making an unchanged valid selection appear lost. Use the same id-based presence check as getGuardrailActionErrorFields.
  const appPickerCtx: GuardrailAppPickerContext = {
    app: action.app.name ? action.app : null,
    onChange: (app) => onChange({ ...action, app: app ?? { id: '', version: '', name: '' } }),

packages/apollo-react/src/canvas/components/Guardrails/components/map-enum-field.tsx:32

  • The fallback repeats the same unchecked unknown default as string[]; a malformed or host-provided non-array sourceDef.defaultValue can therefore flow into keys, and the render below calls keys.map and crashes. Sanitize watchedSelection, sourceSelection, and the definition default as arrays of strings before iterating them.
    (Array.isArray(watchedSelection) ? (watchedSelection as string[]) : undefined) ??
    sourceSelection ??
    (sourceDef?.defaultValue as string[] | undefined) ??
    [];

packages/apollo-react/src/canvas/components/Guardrails/form-schema-builder.ts:139

  • The schema keeps each MetadataForm field name equal to def.id, and apollo-wind uses that name as the DOM id. Consequently, two inline builders for the same definition still emit duplicate input/select ids, so htmlFor and assistive-technology associations can resolve to the first builder. The multiple-instance test explicitly excludes these MetadataForm ids, so this case is not covered. Use a per-instance DOM-id namespace separate from the RHF field name (or add that separation to MetadataForm).
  const initial = initialById.get(def.id);
  const base = {
    name: def.id,
    label: def.label,
    tooltip: def.tooltip,

packages/apollo-react/src/canvas/components/Guardrails/form-schema-builder.ts:233

  • For a multi-value enum whose saved option has since disappeared from def.options, selected still contains the raw value but this options list has no matching label. In the wind MultiSelect, that renders an empty badge with a Remove undefined accessible name, and the chip variant hides the value entirely. Preserve missing selected values as synthetic/raw-labelled options, as the single-value enum path already does, so persisted configurations remain visible and editable.
      return {
        ...base,
        type: 'multiselect',
        placeholder: labels.enumListPlaceholder,
        options: (def.options ?? []).map((opt) => ({
          value: opt,
          label: def.optionLabels?.[opt] ?? opt,

packages/apollo-react/src/canvas/components/Guardrails/form-schema-builder.ts:263

  • defaultValue is typed as unknown, but this cast does not validate it at runtime. If an enum-list definition has a non-array default, sourceSelection becomes that value and MapEnumField later treats it as the selected-key list and calls .map, causing the validator form to throw during render instead of degrading to an empty selection. Normalize this fallback to an actual string array before passing it into the custom field.
      const sourceSelection =
        sourceInitial?.$parameterType === 'enum-list'
          ? sourceInitial.value
          : (sourceDef?.defaultValue as string[] | undefined);

packages/apollo-react/src/canvas/components/Guardrails/guardrail-builder.tsx:63

  • This public prop documentation says the edit value is copied into form state verbatim, but initializeBuilderState now normalizes persisted validator parameters against the definition before rendering. Consumers reading this contract can therefore expect behavior that the implementation no longer provides; document the normalization instead.
  /** Edit mode: initial value, copied verbatim into form state. */

packages/apollo-react/src/canvas/components/Guardrails/guardrail-form-layout.stories.tsx:57

  • This Storybook documentation uses , which violates the repository rule that story copy must not contain spaced em dashes. Replace it with a period or colon so the story passes the documentation convention.
 * Opens on the story canvas but stays closed in docs — see the note on the builder's `EditModal`:

packages/apollo-react/src/canvas/components/Guardrails/guardrail-validator-form.tsx:188

  • When a controlled host removes a previously rendered parameter from parameters, this projection simply omits the field from values. useMetadataFormBridge only synchronizes entries present in that record, so React Hook Form retains the old field value and can continue displaying or submitting a parameter the host removed. Reconcile the set of definition-backed fields and clear/unregister ids absent from the controlled array during synchronization.
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:88
  • This loop only visits keys present in next, so a controlled update that removes a parameter never clears the corresponding React Hook Form field. If a host drops an optional parameter or replaces the array without this id, the old value remains visible and can still be emitted on a later edit, which violates the fully controlled parameters contract. Reconcile the form's registered fields against next and clear or reset ids that are absent.
    packages/apollo-react/src/canvas/components/Guardrails/utils.ts:205
  • MapEnumField intentionally retains rows for keys removed from the sibling enum-list until save, but this public helper scans the raw map. A host following GuardrailValidatorForm's documented save-validation contract can therefore be blocked by an out-of-range value for a key that will be pruned, even though GuardrailBuilder reconciles first. Reconcile map-enum parameters inside this helper, or make the required pre-sync explicit in the public contract, so validation checks the persisted shape.
    packages/apollo-react/src/canvas/components/Guardrails/utils.ts:240
  • The required check also reads the unreconciled map. If a required map's source selection is emptied, stale rows make value look non-empty here, so a standalone host can pass the Save gate even though syncMapEnumParameters will persist an empty map. Apply the same reconciliation used by GuardrailBuilder before this lookup, or document and enforce that ordering for all consumers.
  • Files reviewed: 57/58 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved moderate findings remain across accessibility, controlled-form synchronization, value handling, and list key stability.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (10)

packages/apollo-react/src/canvas/components/Guardrails/README.md:151

  • The README repeats the outdated claim that edit values pass through verbatim. initGuardrailBuilderFormData normalizes persisted parameters before rendering, including correcting the discriminator and malformed list/map shapes, so consumers should not rely on verbatim preservation here.
- `seedGuardrailParameters` builds the initial value array from definitions (editing passes
  the stored values through verbatim), coercing `null` defaults to the union's value types.

packages/apollo-react/src/canvas/components/Guardrails/builder-utils.ts:96

  • This says edit values are copied verbatim, but the existing branch immediately calls normalizeGuardrailParameters, so persisted values are coerced against the definitions before entering form state. Please align this public/internal contract description with the actual normalization behavior.
 * Initial form state: an existing guardrail is copied verbatim (edit); otherwise values are
 * seeded from the definition — parameters via `seedGuardrailParameters`, the scope coerced to
 * the definition's first allowed scope when `Agent` is not allowed, a log/Info default

packages/apollo-react/src/canvas/components/Guardrails/components/escalate-action-fields.tsx:184

  • The recipient-value label has no htmlFor, and both built-in fallback inputs below it have no id, so the visible label is not associated with the control. The searchable/static fallbacks consequently rely on their placeholders for their accessible names instead of the localized recipient label. Give the label and fallback inputs a shared, instance-scoped id association; host slots remain responsible for their own control semantics.
      {/* Recipient value */}
      <FormField>
        <Label>
          {recipientTypeLabels[displayedRecipientType] ?? labels.recipientFallbackLabel}
          <RequiredIndicator />

packages/apollo-react/src/canvas/components/Guardrails/components/map-enum-field.tsx:89

  • When the map has a validation error, the shared FormFieldError is rendered below, but none of these row inputs receives aria-invalid; the invalid styling and assistive-technology state therefore remain absent on the actual controls. Forward the field error state to each row input, as the other form controls do.
            <Input
              aria-label={`${paramDef.label}: ${sourceDef?.optionLabels?.[key] ?? key}`}
              type="number"
              value={currentMap[key] ?? defaults[key] ?? paramDef.min ?? 0}
              onChange={(e) => handleThresholdChange(key, Number.parseFloat(e.target.value) || 0)}
              min={paramDef.min}
              max={paramDef.max}
              step={paramDef.step}
              className="flex-1"

packages/apollo-react/src/canvas/components/Guardrails/components/parameter-label.tsx:25

  • InfoTooltip renders a button, so placing it inside this <Label> violates the label content model and makes clicks on the tooltip ambiguous with activating the associated control. Use the existing FormFieldLabel composition, which renders the tooltip beside the label, or otherwise move the tooltip outside the label.
      {paramDef.tooltip && (
        <InfoTooltip content={paramDef.tooltip} aria-label={labels.moreInformation} />
      )}

packages/apollo-react/src/canvas/components/Guardrails/form-schema-builder.ts:234

  • Only catalog options are supplied for enum-list. If a persisted selection contains an option removed from the current definition, the large MultiSelect renders an unlabeled selected badge (option?.label is undefined), while the small chip editor omits that value entirely. The value remains in selected and is emitted on later edits, so stale data can be persisted with no visible way to manage it. Preserve selected-but-unknown values as synthetic options in both render paths, or explicitly prune/reject them.
        options: (def.options ?? []).map((opt) => ({
          value: opt,
          label: def.optionLabels?.[opt] ?? opt,
        })),

packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:92

  • When a controlled parameters update supplies an invalid value from outside the form, this setValue call does not request validation. With validateLive=true, RHF therefore keeps the previous error state until the user edits the field, so a standalone form can display no error for an externally-set out-of-range or empty value. Pass shouldValidate: validateLive for these sync-in writes (while keeping the gate-off path quiet).
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:93
  • The bridge only writes keys present in next; it never clears a field that the controlled parameters array has removed. After a host reset or an external removal, RHF retains the old value and the UI can submit/re-emit that stale parameter, which violates the documented fully-controlled contract. After syncing the supplied keys, also clear or unregister every schema field absent from next (without touching sidecars).
    packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:183
  • This subscription listens only for value changes, but trigger() on the validateLive transition can overwrite an external host error without changing any value. In the builder, that happens on the first failed Save, so the host's error may disappear immediately even though it still gates Save. Subscribe to error-state changes as well, or explicitly re-apply host errors after trigger() resolves.
    packages/apollo-react/src/canvas/components/Guardrails/utils.ts:33
  • This branch casts every object to Record<string, number> without checking its entries, even though defaultValue is explicitly untrusted. A malformed map default such as { Email: 'bad' } is then read directly by MapEnumField and used as the fallback in syncMapEnumParameters; editing another row or saving can therefore render or emit a non-numeric threshold despite the public value contract. Normalize map entries to finite numbers at the shared boundary before these consumers use them.
  • Files reviewed: 57/58 changed files
  • Comments generated: 3
  • Review effort level: Lite

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

The Guardrails UI, shared by Flow and Agents, as an MUI-free family under
`src/canvas/components/Guardrails`, exported through the narrow
`@uipath/apollo-react/canvas/guardrails` subpath. Members: `GuardrailBuilder`
(the whole Add/Edit screen), `GuardrailFormLayout` (the screen shell), and
`GuardrailValidatorForm` (the validator parameter section).

Built on apollo-wind's forms/ MetadataForm stack rather than its own renderer:
five of the seven parameter types map onto first-class field types, while the
chip-style enum-list, `map-enum` and host `renderParameter` overrides register
as custom components. Strings localize through lingui (`guardrails.*` ids,
14 catalogs).

MetadataForm owns its own state, so this family's controlled contract is
translated onto its plugin seam in exactly one named place,
`useMetadataFormBridge`: it registers the custom components from the first
paint, pushes host values in structurally compared (so an echo of the form's
own emission performs no write and focus survives), pushes host errors in as
`type: 'external'`, and suppresses its own echo while writing. A sync arriving
before `onFormInit` is replayed rather than dropped.

Validation is shared, and the split is deliberate: the schema declares
`required`/`min`/`max` from the definitions with messages from the label
catalog, so they translate; the host owns domain rules and the save-time gate
through `getRequiredEmptyParameterIds` / `getOutOfRangeParameterIds`. Where the
two disagree the host's verdict is what renders — a `text-list` of
whitespace-only rows passes the array's `.min(1)` but counts as empty for the
host predicate — and a test pins that. Custom fields declare a `valueType` so
those constraints bind to them too; `map-enum` has no counterpart shape, so its
required check stays the host's alone.

Save-time correctness: the builder validates the *reconciled* parameters, since
the editor deliberately keeps rows for deselected map keys while the save path
prunes them — reading the raw values let a required map look filled by a stale
key and then persist as `{}`. Host parameter errors drop out of the display and
the Save gate once the user edits past them, so a backend verdict no longer
outlives the edit that fixed it.

`onSaveAsNew` hands the host the original selector verbatim, scopes included.
Narrowing a duplicated guardrail's coverage is a product decision with safety
consequences, so it is a documented contract with the transformation left to
the host rather than an undocumented accident.

Single-line inputs the builder renders itself (name, escalation recipient/app
fallbacks) sit outside the nested MetadataForm's guarded div, so the root
swallows Enter for them too: mounted `inline` inside a host form, Enter would
otherwise trigger the host's implicit submission and skip `handleSave`.

Host errors are re-asserted after the form revalidates: the bridge checks the
field's real error state rather than the last message it wrote, and a values
subscription restores anything the resolver replaces, so the prop stays the
source of truth.

Control ids are namespaced per instance with `useId`, so two inline builders can
share a document without `htmlFor` binding a label to the wrong one, and a
required map-enum surfaces its label and error even when the source selection is
empty and there are no rows to show.

Review round from BenGSchulz: malformed persisted values no longer crash the
builder or slip past the Save gate (one `coerceParameterValueToType` applied to
defaults and wire data alike); `overrideParameterIds` lets a host declare which
parameters it overrides instead of the form probing `renderParameter` on every
render; the chip groups carry a real accessible name; the escalation slots agree
on one error-ownership rule; FieldShell and the chips track apollo-wind's
`future:` layer; and the modal stories no longer open over the docs page.

Malformed wire data is normalised against the definitions — value *and*
discriminator — while host sidecars with no definition pass through untouched,
and a non-finite persisted map threshold falls back rather than reaching onSave.

QA round: the dialog validated on two timings. Parameter fields reported the
moment they were edited, while name, scopes and action waited for a Save
attempt — clearing a multi-select went red on the spot, clearing every scope
said nothing. The resolver now runs behind the same gate, through a
`validateLive` prop that `GuardrailBuilder` drives from its own
post-save-attempt flag. It defaults to on, so a host mounting
`GuardrailValidatorForm` standalone, with no save of its own, still validates as
before. Opening the gate revalidates explicitly rather than waiting for the next
keystroke, so the messages land on the Save that revealed them.

Copilot round: the enum-list chip group now carries the same accessible naming as the
scope selector (role=group + aria-labelledby, aria-invalid and aria-describedby onto
the error), since the chips are Toggle buttons and the bare label named nothing; the
mixed-scopes banner prefixes its list keys by source, because a tool may legitimately
be named after a scope and both lists render as siblings of one <ul>; and the form
layout's Modal story drops its spaced em dash.

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unresolved moderate accessibility, form-submission, and controlled-state issues remain, alongside two documentation nits.

Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file

Suppressed comments (7)

packages/apollo-react/src/canvas/components/Guardrails/README.md:151

  • This public API description says edit values pass through verbatim, but seedGuardrailParameters calls normalizeGuardrailParameters for existing data and can coerce malformed values and replace a mismatched $parameterType. Consumers relying on this statement may assume persisted values are preserved when the component intentionally changes them; document the normalization behavior instead.
- `seedGuardrailParameters` builds the initial value array from definitions (editing passes
  the stored values through verbatim), coercing `null` defaults to the union's value types.

packages/apollo-react/src/canvas/components/Guardrails/builder-utils.ts:97

  • This JSDoc repeats that an existing guardrail is copied verbatim, but the edit path calls normalizeGuardrailParameters and can change known parameter values and discriminators. The comment should describe normalization so the implementation and exported helper documentation do not promise a behavior that is not provided.
 * Initial form state: an existing guardrail is copied verbatim (edit); otherwise values are
 * seeded from the definition — parameters via `seedGuardrailParameters`, the scope coerced to
 * the definition's first allowed scope when `Agent` is not allowed, a log/Info default
 * action, and evaluations enabled.

packages/apollo-react/src/canvas/components/Guardrails/components/escalate-action-fields.tsx:185

  • The visible recipient label is not associated with either built-in fallback input: it has no htmlFor, and both fallback <Input> elements have no id. Screen readers therefore do not receive the recipient type label for the search, email, or group-name control. Generate a stable control id and pair it with the label, while retaining the host-slot escape hatch.
        <Label>
          {recipientTypeLabels[displayedRecipientType] ?? labels.recipientFallbackLabel}
          <RequiredIndicator />
        </Label>

packages/apollo-react/src/canvas/components/Guardrails/components/map-enum-field.tsx:82

  • These numeric controls render the field-level error below them, but unlike the other parameter controls they never expose aria-invalid. A user of assistive technology therefore cannot detect that a threshold input is invalid when the host or range validation reports an error. Forward the invalid state to each row input (and associate it with the field error if possible).
              aria-label={`${paramDef.label}: ${sourceDef?.optionLabels?.[key] ?? key}`}

packages/apollo-react/src/canvas/components/Guardrails/guardrail-form-layout.tsx:75

  • In inline mode this layout is intended to be embedded in a host <form>, but none of these footer buttons sets type="button". Button forwards the native button unchanged, so Cancel, Save as new, and Save default to submit and also submit the host form when clicked. Set all footer buttons to type="button" so only their explicit callbacks run.
      <Button variant="outline" size={size} onClick={onCancel}>
        {cancelLabel}
      </Button>
      {secondaryAction && (
        <Button
          variant="outline"
          size={size}
          onClick={secondaryAction.onClick}
          disabled={secondaryAction.disabled}
        >
          {secondaryAction.label}
        </Button>
      )}
      <Button size={size} onClick={onSave} disabled={saveDisabled}>

packages/apollo-react/src/canvas/components/Guardrails/guardrail-form-layout.tsx:89

  • The inline back button can also be rendered inside a host <form>, but it has no explicit button type and therefore defaults to submit. Clicking the header's Cancel/back control can trigger the host form submission in addition to onCancel; give it type="button".
            <Button variant="ghost" size="2xs" icon aria-label={cancelLabel} onClick={onCancel}>

packages/apollo-react/src/canvas/components/Guardrails/use-metadata-form-bridge.ts:94

  • When a parameter is removed from the controlled parameters array, values no longer contains its id, but this loop only updates keys that are present in next. The corresponding RHF field therefore keeps displaying its previous value, and editing that stale control immediately upserts the removed parameter again through onValueChange, violating the fully controlled contract. Track previously synchronized ids and explicitly clear/reset ids absent from the new host snapshot.
  • Files reviewed: 57/58 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dev-packages Adds dev package publishing on pushes to this PR pkg:apollo-react size:XXL 1,000+ changed lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants