feat(apollo-react): guardrail remove confirmation dialog [AL-577] - #1149
andreizdrali-uipath wants to merge 2 commits into
Conversation
|
Apollo Coded App preview deployments are ready.
|
Dependency License Review
License distribution
Excluded packages
|
📦 Dev Packages
|
There was a problem hiding this comment.
🟡 Changes recommended
The dialog list keys can collide and must be made unique before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a shared Apollo React guardrail removal confirmation dialog with structured impact messaging, localization, controlled lifecycle, and accessibility support.
Changes:
- Added the dialog, stories, tests, and public exports.
- Added localized labels and catalog parity coverage.
- Documented guardrail removal behavior and integration contracts.
File summaries
| File | Review summary |
|---|---|
pnpm-lock.yaml |
Reviewed; no final comment. |
packages/apollo-wind/src/index.ts |
Reviewed; no final comment. |
packages/apollo-wind/src/components/ui/textarea.tsx |
Reviewed; no final comment. |
packages/apollo-wind/src/components/ui/select.tsx |
Reviewed; no final comment. |
packages/apollo-wind/src/components/ui/info-tooltip.tsx |
Reviewed; no final comment. |
packages/apollo-wind/src/components/ui/info-tooltip.test.tsx |
Reviewed; no final comment. |
packages/apollo-wind/src/components/ui/index.ts |
Reviewed; no final comment. |
packages/apollo-wind/src/components/forms/validation-converter.ts |
Reviewed; no final comment. |
packages/apollo-wind/src/components/forms/string-list-field.tsx |
Reviewed; no final comment. |
packages/apollo-wind/src/components/forms/metadata-form.stories.tsx |
Reviewed; no final comment. |
packages/apollo-wind/src/components/forms/index.ts |
Reviewed; no final comment. |
packages/apollo-wind/src/components/forms/form-schema.ts |
Reviewed; no final comment. |
packages/apollo-wind/src/components/forms/field-renderer.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/test/setup.ts |
Reviewed; no final comment. |
packages/apollo-react/src/i18n/index.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/index.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/utils.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/use-guardrail-definitions.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/types.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/render-parameter-bridge.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/index.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/i18n.test.ts |
Reviewed catalog parity coverage; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/guardrail-validator-form.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/guardrail-remove-dialog.tsx |
Moderate (2 votes): raw list keys can collide across item kinds or repeated values; use unique keys. |
packages/apollo-react/src/canvas/components/Guardrails/guardrail-remove-dialog.test.tsx |
Reviewed dialog behavior and accessibility coverage; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/guardrail-remove-dialog.stories.tsx |
Reviewed dialog stories; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/guardrail-form-layout.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/guardrail-form-layout.test.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/guardrail-form-layout.stories.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/form-schema-builder.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/definitions-wire.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/definitions-parse.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/definitions-parity.test.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/definitions-copy.test.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/parameter-label.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/mixed-scopes-banner.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/mixed-scopes-banner.test.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/map-enum-field.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-status-banner.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-status-banner.test.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-scope-selector.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-scope-selector.test.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-chip.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-chip.test.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-action-section.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-action-section.test.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/field-shell.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/field-shell.test.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/components/enum-list-chips-field.tsx |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/builder-utils.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/builder-utils.test.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/builder-types.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/__fixtures__/host-copy-baselines.ts |
Reviewed; no final comment. |
packages/apollo-react/src/canvas/components/Guardrails/__fixtures__/definitions-wire.fixtures.ts |
Reviewed; no final comment. |
packages/apollo-react/package.json |
Reviewed; no final comment. |
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 85/86 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
📊 Coverage + size by packagePer-package coverage and bundle size on this PR. New-line coverage = of the source lines this PR adds or changes, the % hit by tests.
"Coverage" is each package's own |
There was a problem hiding this comment.
🔵 Needs a closer look
Three moderate findings remain regarding impact ordering, list-key uniqueness, and the ru catalog orphan scan.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
packages/apollo-react/src/canvas/components/Guardrails/guardrail-remove-dialog.tsx:85
ImpactListrenderstoolNamesbeforescopes, but the dialog contract requires scopes to precede tools. WithaffectedToolNames={['Send email']}andaffectedScopes={['Llm']}, this still producesSend email, LLM callsinstead of the promised scope-first order; render the scope items before the tool items and update the order assertions.
packages/apollo-react/src/canvas/components/Guardrails/guardrail-remove-dialog.tsx:89
- These keys come from two independent arrays rendered as siblings in the same
<ul>. A tool namedAgent,Llm, orToolcan therefore collide with a scope key (and repeated values collide within either array), producing duplicate-key warnings and potentially stale item reconciliation when the impact changes. Prefix the keys by source and include an index, or otherwise generate unique keys for both maps.
packages/apollo-react/src/canvas/components/Guardrails/i18n.test.ts:107 - The orphan-id check claims to cover every catalog, but this locale list omits
ru, which is still a checked-in catalog. A staleguardrails.remove-dialog.*entry inru.jsonwould pass unnoticed; includeruin this orphan scan while keeping it out of the translated-coverage check because it intentionally falls back to English.
- Files reviewed: 85/86 changed files
- Comments generated: 0 new
- Review effort level: Lite
Storybook visual diffBaseline is the deployed main Storybook, so changes merged to main after this branch was last updated can also appear here. Logs Updated (PT): Sep 17, 2026, 04:44:55 AM |
7db5698 to
d816880
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The dialog currently renders tool names before scopes; update the component and its test before approval.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/apollo-react/src/canvas/components/Guardrails/guardrail-remove-dialog.tsx:90
- The dialog contract says scopes should appear before tool names in the impact list, but this renders
toolNamesfirst andscopessecond. Once the adapters pass the corrected arrays, users will still see the list in the wrong order (andguardrail-remove-dialog.test.tsxcurrently locks in that order); render the scopes before the tools and update the expectation.
- Files reviewed: 102/103 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate dialog issues remain: impact ordering and duplicate list keys.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
packages/apollo-react/src/canvas/components/Guardrails/guardrail-remove-dialog.tsx:96
- The impact list is rendered in tool-first order, but the corrected host contract calls for scopes before tools. With
affectedScopes={['Llm']}andaffectedToolNames={['Send email']}, this producesSend email, thenLLM callsinstead of the intended scope-first order; the story/test currently encode that regression as well. Render the scope entries before the tool entries and update the expected order.
packages/apollo-react/src/canvas/components/Guardrails/guardrail-remove-dialog.tsx:95 toolNamesandscopesshare the same<ul>, but their keys use the sameindex-valueformat. If a tool name equals a scope at the same index (for example, a tool namedAgentalongside theAgentscope), React emits duplicate-key warnings and can reconcile the wrong<li>when the impact changes. Prefix the key by list kind so the two namespaces cannot collide.
- Files reviewed: 102/103 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Two unresolved moderate findings remain: add overlay non-dismissal coverage and reconcile the required destructive confirm variant.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
packages/apollo-react/src/canvas/components/Guardrails/guardrail-remove-dialog.tsx:141
- The component contract documents that backdrop/outside clicks must not dismiss this destructive confirmation, but the dialog suite only covers Cancel and Escape. Please add an interaction test that clicks the overlay and asserts
onCancelis not called and the alert dialog remains open; otherwise a Radix/wind behavior change could silently reintroduce dismissal.
packages/apollo-react/src/canvas/components/Guardrails/guardrail-remove-dialog.tsx:191 - This implementation intentionally renders the remove action with the default accent variant, but the PR's stated “What ships” contract and QA-visible behavior require
Button variant="destructive". Please reconcile the shipped behavior with the PR/design decision before merge: either restore the destructive variant or update the description and acceptance criteria to the accent behavior.
- Files reviewed: 102/103 changed files
- Comments generated: 0 new
- Review effort level: Lite
88b9474 to
35f022d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The claimed localized dialog behavior is not delivered because non-English catalogs lack the new IDs and the localization test runs only with the English locale.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 27/27 changed files
- Comments generated: 2
- Review effort level: Lite
35f022d to
3e03845
Compare
3e03845 to
7dc4948
Compare
7dc4948 to
5d281a7
Compare
5d281a7 to
6ecfa96
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The shared destructive-removal flow changes dismissal behavior and needs final product confirmation of the documented Agents and scope-formatting decisions.
Review details
- Files reviewed: 27/27 changed files
- Comments generated: 0 new
- Review effort level: Lite
6ecfa96 to
e853347
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The implementation uses a plain dialog with a default corner close button, conflicting with the stated AlertDialog contract and Agents dismissal behavior.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 27/27 changed files
- Comments generated: 1
- Review effort level: Lite
Add `GuardrailRemoveDialog`, the confirmation step before a guardrail is removed, with the impact of the removal spelled out: what a full removal also takes the guardrail off, or what survives when only one tool is detached. Both products already ask the same question about those same two situations and differ only in how they compute them, so the impact arrives as four structured props (`affected*`, `remaining*`) rather than a content slot. Flow's `isToolOnAgent` branch converges here by filling one pair or the other. Pass neither and the dialog is the question on its own. Deciding which removal is happening, and unwinding it, stays host-side: Agents strips the tool from `matchNames`, drops the `Tool` scope and then deletes, Flow calls `removeToolFromGuardrail`, and neither belongs in the package. Both callbacks are intents, so the write, the telemetry and the close stay with the host and `open` stays controlled. Built on wind's `Dialog` rather than `AlertDialog`, to match the rest of the family. `AlertDialog` carries `role="alertdialog"` and focuses its cancel control for free, but it has no corner close button and no way to add backdrop dismissal: Radix prevents outside interaction after spreading consumer props, so neither is reachable. `GuardrailFormLayout` is already a wind `Dialog` with a close button, and both products' own remove dialogs offer some way out of the corner, so consistency won. What the switch costs is made back explicitly: `onOpenAutoFocus` names Cancel so initial focus is a decision rather than a consequence of markup order, and the description still carries the whole impact through `aria-describedby`. `showCloseButton` and `closeOnBackdropClick` are both opt-in, off by default. Adopting the dialog should not add a control a product did not have, and a confirmation is a decision that a stray click on the backdrop should not discard; Escape and Cancel always dismiss. Agents asks for the close button, Flow leaves it off. Wind's prop of the same name only applies to `variant="takeover"`, so the guard here is `onPointerDownOutside`. Wind's own built-in close button is suppressed because its accessible name is a hardcoded English "Close", which would be the only untranslated string in the family; the replacement takes its label from the catalog and sits last in the DOM, so a keyboard user reaches the decision before the escape hatch. Two things the host dialogs get wrong today are fixed by construction. The confirm button is a plain `Button`, not a close: closing from inside would report a cancel with every confirm, which Flow's dialog does through `AlertDialogAction`. And `DialogDescription` takes `asChild` over a `<div>`, so the impact blocks are part of the description the dialog announces instead of `<p>`/`<ul>` nested inside a `<p>`. A third becomes a one-line fix rather than being fixed here: Agents' listing section passes its two impact arrays swapped, which the typed props make obvious but do not correct, since the call site is not this package's. The confirm button takes the default accent variant. Both products colour this action with the accent today and both read a red Remove as a regression when they tested the shared dialog; the dialog carries the weight of the action through its title and impact lines instead. A test pins it. Scopes arrive as raw wire values and localize through `formatScope`, the same idiom the list section uses: scope vocabulary is product-owned and both products already hold the mapping. `container` picks the portal target, so Agents keeps the `document.body` escape its shadow-root host needs while Flow's dialog stays in place. Eight `guardrails.remove-dialog.*` lingui ids. Seven are harvested from the two products' own catalogs where the English already matched in both, and `close` comes from Agents' `common.close`, so none of this copy is newly invented here. English only, like the rest of the package: `chore(l10n): sync from Localization` owns the other thirteen catalogs. The i18n test uses the family's shared catalog scans (`__fixtures__/catalog-coverage`), and `mergeLabels` is the same helper the sibling list and palette branches add, byte-identical so the three of them merge without a conflict. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
e853347 to
1173f1e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
showCloseButton defaults to false even though the PR contract says it must default to true for Flow and Agents.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 27/27 changed files
- Comments generated: 1
- Review effort level: Lite
AL-577, the remove-guardrail confirmation dialog: the step both products show before a guardrail
is removed. Fourth member of the shared family in
packages/apollo-react/src/canvas/components/Guardrails/.One commit over #1139, which is one commit over
main. Review the last commit only.What ships
guardrail-remove-dialog.tsxGuardrailRemoveDialog,GuardrailRemoveDialogPropsi18n.ts(extended)GuardrailRemoveDialogLabels,GUARDRAIL_REMOVE_DIALOG_EN_LABELS,GUARDRAIL_REMOVE_DIALOG_EN_MESSAGES,resolveGuardrailRemoveDialogLabels,useGuardrailRemoveDialogLabelsPlus 5 stories, 20 component tests,
i18n.test.ts, a README section and 8guardrails.remove-dialog.*ids inen.json.Contract
The host decides which removal is happening and unwinds it; the dialog renders the description of
the outcome. Agents strips the tool from
matchNames, drops theToolscope and then deletes;Flow calls
removeToolFromGuardrail. Both callbacks are intents, so the write, the telemetry andthe close stay with the product and
openstays controlled.Design decisions
affected*is what a full removalalso takes the guardrail off,
remaining*is what survives a tool-scoped one. Flow'sisToolOnAgentbranch converges by filling one pair or the other; all four default to empty.Dialog, notAlertDialog, so a corner close button is possible at all.AlertDialoghasnone and cannot be given backdrop dismissal either, since Radix prevents outside interaction
after spreading consumer props, so offering either would have meant working against the
primitive.
GuardrailFormLayoutis already a windDialogwith a close button. Cost:role="alertdialog"becomesrole="dialog", andonOpenAutoFocusrestores the focus-on-Cancelthat came free.
showCloseButtonandcloseOnBackdropClickare both opt-in. Adopting the dialog should notadd a control a product did not have, and a confirmation is a decision that a stray click should
not discard; Escape and Cancel always dismiss, and everything routes through one
onCancelpath. Wind's
closeOnBackdropClickis takeover-only, so the guard isonPointerDownOutside,and wind's own close button is always suppressed because its accessible name is a hardcoded
English "Close".
Button, not aDialogClose. Closing from inside reports a cancelwith every confirm, which Flow does today through
AlertDialogAction.destructive. Both products colour this action with theaccent today and read a red Remove as a regression, so neither button changes colour. A test
pins it. feat(apollo-react): guardrail list section [AL-575] #1140's
text-destructiverow button is still consistent: the rule both productsfollow is destructive tint on a ghost trigger, never a filled destructive button.
asChildonDialogDescription, whichalso drops the invalid
<p>/<ul>-inside-<p>nesting Flow has today.formatScope, matchingGuardrailList'sformatScopes. Tools list before scopes, pinned by a test.containerpicks the portal target,'body'forcingdocument.bodyeven under aPortalContainerProvider, which is what preserves Agents' escape from its shadow root.onTrack, despite Confluence §7.5, same call as feat(apollo-react): guardrail list section [AL-575] #1140: the two event taxonomies do notoverlap and both adapters wrap the intents anyway.
QA-visible per host
showCloseButtonto keep its corner close button, andcloseOnBackdropClickto keep backdrop dismissal. Initial focus moves from that button to Cancel.
GuardrailsListingSection.handleRemoveGuardrailpasses its two impactarrays swapped, so its dialog shows scopes before tools with
Llmraw instead of "LLM calls".The typed props make that a one-line fix but do not apply it, since the call site is Agents-side.
Strings
Eight
guardrails.remove-dialog.*ids in one builder function, thedefinitions-copy.tsidiom,added to
en.jsononly. Seven exist in both products with identical English and the eighth,close, comes from Agents'common.close, so none of this copy is newly written here.The other thirteen catalogs are
chore(l10n): sync from Localization's, and until it runsuseSafeLinguirenders the English default, so nothing is missing on screen.i18n.test.tsasserts English parity with the source through #1139's shared catalog scans.
Verification
Guardrails suite 415 passing, jest-axe plus the focus trap, Escape, both opt-in dismissals and
the portal
container.tsc --noEmit,biome lint src,biome format srcand therslib build clean; no icon churn, no
pnpm-lock.yamlchange. Tests and stories typechecked with athrowaway tsconfig, since the package one excludes both. Also checked in a browser at the URL the
visual-diff job snapshots: focus on Cancel, tab order Cancel then Remove then Close once the
close button is on, a backdrop pointer-down leaving the dialog open, and the docs page holding no
dialog and an interactive
body.mergeLabelsini18n.tsis the only pre-existing code touched: a pure refactor of the twoexisting resolvers onto one generic merge, byte-identical to the helper #1140 and #1147 each
add, so the three branches collapse onto one copy instead of four.
Open question
Should
formatScopedefault to the family's own scope labels? Family-wide rather than thisPR's: #1140 and #1149 render raw with an optional formatter, #1147 shows no scopes, and #1161
defaults scope, action and execution-stage labels. The split is 2-1 with the newer decision on
the defaulting side. Following #1161 here is cheap, three label fields reusing the existing
guardrails.builder.scope-*-labelids so no new catalog entries, but it should change #1140,#1149 and #1161 together or not at all.
🤖 Generated with Claude Code