feat(apollo-wind): polish field and inventory patterns - #1113
Conversation
|
Apollo Coded App preview deployments are ready.
|
Dependency License Review
License distribution
Excluded packages
|
There was a problem hiding this comment.
🟡 Changes recommended
A few newly introduced behaviors have concrete API/UX/a11y issues (e.g., showLock suppressing custom leading addons, clickable menu items without handlers, and removed focus-visible ring on read-only inputs) that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates Apollo UI field/input interaction patterns and Storybook examples, primarily in @uipath/apollo-wind, and extends apollo-react canvas stories to showcase the refreshed patterns (including deep-linkable UI Inventory sections).
Changes:
- Extend
LockableValueFieldwith configurable lock visibility, optional AI-assist visibility, and an optional “More actions” overflow menu. - Refine input and dropdown styling/behavior (read-only treatment, Future theme field trigger sizing, destructive menu item color), and update component stories accordingly.
- Add deep-linking + copy-link behavior for UI Inventory tabs/sections in the
apollo-reactNodePropertyPanel Storybook story.
File summaries
| File | Description |
|---|---|
| packages/apollo-wind/src/index.ts | Re-export newly added LockableValueField type. |
| packages/apollo-wind/src/components/ui/lockable-value-field/types.ts | Add showLock, showAiAssist, and more props/types for LockableValueField. |
| packages/apollo-wind/src/components/ui/lockable-value-field/lockable-value-field.tsx | Implement “More actions” menu and new visibility props; adjust locked rendering path to use InputGroup. |
| packages/apollo-wind/src/components/ui/lockable-value-field/lockable-value-field.test.tsx | Add tests for showLock, showAiAssist, and “More actions” rendering. |
| packages/apollo-wind/src/components/ui/lockable-value-field/lockable-value-field.stories.tsx | Add a new Storybook demo showcasing “More actions”. |
| packages/apollo-wind/src/components/ui/lockable-value-field/index.ts | Export LockableValueFieldMoreActions type from the component entrypoint. |
| packages/apollo-wind/src/components/ui/lockable-value-field/components/field-header.tsx | Gate the AI-assist UI via new showAiAssist prop (while keeping other field actions). |
| packages/apollo-wind/src/components/ui/input.tsx | Update Input styling for read-only state behavior. |
| packages/apollo-wind/src/components/ui/input.stories.tsx | Add a read-only Input story and guidance copy. |
| packages/apollo-wind/src/components/ui/dropdown-menu.tsx | Adjust Future theme field trigger sizing. |
| packages/apollo-wind/src/components/ui/dropdown-menu.stories.tsx | Add “more actions” example and update destructive menu item color token usage. |
| packages/apollo-wind/src/components/ui/combobox.tsx | Update combobox trigger icon. |
| packages/apollo-wind/src/components/ui/combobox.stories.tsx | Update combobox stories to match the new icon. |
| packages/apollo-wind/src/components/ui/button.stories.tsx | Add a Button catalog example for a lightweight “+ Add field” link action. |
| packages/apollo-react/src/canvas/stories/templates/Flow.stories.tsx | Update DAP connection field to use LockableValueField and “More actions” behavior. |
| packages/apollo-react/src/canvas/components/NodePropertyPanel/NodePropertyPanel.stories.tsx | Add deep-linking and copy-link controls for UI Inventory tabs/sections; swap removal icons to trash icons in several spots. |
Review details
Suppressed comments (2)
packages/apollo-wind/src/components/ui/lockable-value-field/lockable-value-field.tsx:161
showLockcurrently gates the entire leading addon region (showLock && leadingAddon !== null), so consumers cannot hide the built-in lock toggle while still rendering a customleadingAddon. This contradicts the prop doc (“built-in lock control”) and breaks the existingleadingAddonoverride semantics.
This issue also appears on line 318 of the same file.
{showLock && leadingAddon !== null && (
<InputGroupAddon align="inline-start">
{leadingAddon !== undefined ? (
leadingAddon
) : (
packages/apollo-wind/src/components/ui/lockable-value-field/lockable-value-field.tsx:322
- Same issue in the non-expression branch:
showLockcurrently suppresses any customleadingAddoncontent because it gates the whole addon wrapper.showLockshould only control the default lock toggle, not consumer-provided addons.
{showLock && leadingAddon !== null && (
<InputGroupAddon align="inline-start">
{leadingAddon !== undefined ? (
leadingAddon
) : (
- Files reviewed: 16/16 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Addressed the review feedback in commit
Validation: 1,437 tests passed; focused Biome and diff checks passed. |
📊 Coverage + size by packagePer-package coverage and bundle size on this PR. New-line coverage = of the source lines this PR adds or changes, the % hit by tests.
"Coverage" is each package's own |
There was a problem hiding this comment.
🟡 Changes recommended
The new deep-link/copy-link and “More actions” implementations have a couple of concrete edge-case bugs (unhandled clipboard failures, hash parsing robustness, and More actions respecting optional handlers/locked state) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
packages/apollo-react/src/canvas/components/NodePropertyPanel/NodePropertyPanel.stories.tsx:3871
syncFromHashusesdecodeURIComponentand then blindly applies the parsed tab value. A malformed percent-encoding can throw, and an unknown tab hash can leave Tabs in an invalid state (no content). Guard against decode errors and validate the tab before updating state.
packages/apollo-wind/src/components/ui/lockable-value-field/lockable-value-field.tsx:46
LockableValueFieldMoreActionsmarksonClear/onRefreshas optional, butMoreActionsMenualways renders both items and wires them directly. This can produce menu items that do nothing, and it also lets "Clear value" run even when the field islocked(read-only), which contradicts the component’s locked/read-only behavior.
function MoreActionsMenu({ more }: { more: LockableValueFieldMoreActions }) {
return (
<DropdownMenu>
<DropdownMenuTrigger asChild>
<InputGroupButton icon size="3xs" aria-label="More value actions">
packages/apollo-wind/src/components/ui/lockable-value-field/lockable-value-field.tsx:382
- Same as above: the non-expression branch renders
MoreActionsMenuwithout passing whether the field is locked, so "Clear value" can still be offered in a read-only state.
{more && (
<InputGroupAddon align="inline-end" className="cursor-default">
<MoreActionsMenu more={more} />
</InputGroupAddon>
packages/apollo-react/src/canvas/components/NodePropertyPanel/NodePropertyPanel.stories.tsx:3545
navigator.clipboard.writeTextcan throw (permission denied, insecure context, missing API), which would create an unhandled promise rejection in Storybook. The copy-link helper should handle failures gracefully.
const copyLink = async () => {
if (!linkTarget) return;
const url = new URL(window.location.href);
url.hash = `ui-inventory/${linkTarget}`;
window.history.replaceState(null, '', url);
await navigator.clipboard.writeText(url.toString());
setCopied(true);
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
A few accessibility/UX regressions were introduced in Storybook patterns (read-only fields removed from tab order) and minor ARIA/cursor inconsistencies should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/ui/input.stories.tsx:92
- The ReadOnly Input story sets
tabIndex: -1, which removes the control from keyboard navigation. For a read-only field this makes it harder for keyboard and assistive-technology users to discover and copy the value; keeping the default tab order is typically preferable.
packages/apollo-wind/src/components/ui/lockable-value-field/lockable-value-field.tsx:351 InputGroupInputalready derivesaria-invalid,aria-describedby, andaria-errormessagefrom the surroundingInputGroup'serror/errorIdcontext. Passing those props here can duplicate the error id inaria-describedbyand makes the attributes easier to get out of sync; it should be enough to rely onInputGroupInput's built-in wiring.
- Files reviewed: 16/16 changed files
- Comments generated: 3
- Review effort level: Lite
|
Resolved the remaining review findings in commit
Validation: 1,439 tests passed; Biome and diff checks passed. New CI checks are running. |
|
Resolved the latest accessibility review findings in commit
Validation: 1,439 tests passed; Biome and diff checks passed. |
There was a problem hiding this comment.
🔵 Needs a closer look
The UI Inventory hash-sync currently auto-expands Accordion state for any section hash (including non-Accordion targets), which can desync the controlled Accordion value from its actual item set.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packages/apollo-react/src/canvas/components/NodePropertyPanel/NodePropertyPanel.stories.tsx:3881
syncFromHashadds anysectiontoexpandedSections, including non-accordion targets likeflat-content, which can desync the Accordion controlled value from its actual item list. Restrict the auto-expand behavior to known Accordion sections only.
packages/apollo-wind/src/components/ui/lockable-value-field/types.ts:139- The
moreprop doc says the overflow menu is beside the fixed/expression control, but the component also renders it for field types that don't support expressions (where no mode control exists). Updating the comment will prevent confusion for consumers.
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
A few new Storybook usages introduce accessibility and lifecycle issues (custom labels missing htmlFor/id association and an uncleared timeout that can update state after unmount).
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
packages/apollo-react/src/canvas/components/NodePropertyPanel/NodePropertyPanel.stories.tsx:3552
copyLinkschedulessetCopied(false)viasetTimeoutwithout any cleanup, which can trigger a state update after the story unmounts (e.g., when switching stories/tabs quickly) and cause React warnings. Consider moving the timeout into an effect that cleans up on unmount / whencopiedchanges.
packages/apollo-react/src/canvas/stories/templates/Flow.stories.tsx:3860- The custom
labelReactNode passed toLockableValueFieldis rendered as-is, so omittinghtmlForon the<Label>breaks label-to-control association for screen readers. AddhtmlForthat matches theidyou pass to the field.
packages/apollo-wind/src/components/ui/lockable-value-field/lockable-value-field.stories.tsx:127 - In this story the
labelis a custom ReactNode and is rendered as-is byFieldHeader, so without anid+ matchinghtmlForthe label isn't associated with the control. Add anidand passhtmlForon the<Label>(matching the pattern used inDefaultDemo).
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Resolved the latest review findings in commit
Validation: 1,439 tests passed; Biome and diff checks passed. |
|
Resolved the latest review findings in commit
Validation: 1,439 tests passed; Biome and diff checks passed. |
There was a problem hiding this comment.
🟡 Changes recommended
The new “Clear value” more-actions UI still uses an X icon (and missing destructive styling in one spot), which conflicts with the PR’s stated iconography and destructive-action color patterns.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/ui/dropdown-menu.stories.tsx:24
- After switching the Clear value action to use Trash2, the X icon import becomes unused and should be removed to avoid lint/typecheck failures.
This issue also appears on line 129 of the same file.
packages/apollo-wind/src/components/ui/dropdown-menu.stories.tsx:131
- In the new “Dropdown with more actions” example, the Clear value action still uses the X icon and default text styling. The PR description calls for using trash icons for removal-style actions and the inline-validation error color for destructive menu actions; this example should match that pattern.
<X />
Clear value
</DropdownMenuItem>
packages/apollo-wind/src/components/ui/lockable-value-field/lockable-value-field.tsx:65
- The new LockableValueField more-actions “Clear value” entry still uses the X icon. The PR description says removal-style X icons should be replaced with trash icons, so this should use Trash2 (and update the lucide-react import accordingly).
<X />
Clear value
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
|
Resolved the latest iconography review finding in commit
Validation: 1,439 tests passed; Biome and diff checks passed. |
There was a problem hiding this comment.
🔵 Needs a closer look
DropdownMenuTrigger’s new future:w-fit field styling can unexpectedly override consumer width expectations in Future theme and should be adjusted before merging.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/ui/dropdown-menu.tsx:50
future:w-fiton thefieldstyling makes the trigger shrink-to-content in Future theme even when consumers passw-full(they would need afuture:w-fulloverride). Sincefieldimplies form-control behavior, this width override is surprising and can break full-width layouts.
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Resolved the latest Dropdown review finding in commit
Validation: 1,439 tests passed; Biome and diff checks passed. |
There was a problem hiding this comment.
🟢 Approval recommended
The new props/behaviors are implemented consistently, covered by targeted tests, and the remaining changes are Storybook/demo-focused with low risk to runtime consumers.
Review details
- Files reviewed: 15/15 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 02, 2026, 12:37:39 PM |
There was a problem hiding this comment.
🟢 Approval recommended
Changes are cohesive UI/demo refinements with added component capabilities and accompanying unit test coverage for the new LockableValueField behaviors.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, include test coverage for the new LockableValueField behaviors, and appear consistent with existing component patterns and Storybook usage.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
InputGroup integration for embedded controls is incomplete (notably focus-ring ownership for Select/MultiSelect), which can cause inconsistent or incomplete keyboard focus indication around grouped controls.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/ui/lockable-value-field/lockable-value-field.tsx:363
- When embedding SelectTrigger inside InputGroup, it should be treated as the group “control” so the wrapper can render the unified focus ring. InputGroup’s focus styling keys off
data-slot="input-group-control"(see input-group.tsx:47/170), but SelectTrigger here doesn’t set it, so focus styling won’t be consistent with other InputGroup controls and the trigger may draw its own ring independently.
This issue also appears on line 375 of the same file.
<SelectTrigger
id={fieldId}
className="min-w-0 flex-1 rounded-none border-0 bg-transparent px-0 shadow-none future:rounded-none future:border-0 future:bg-transparent future:px-0"
aria-invalid={error ? true : undefined}
aria-describedby={error ? validationId : undefined}
aria-errormessage={error ? validationId : undefined}
>
packages/apollo-wind/src/components/ui/lockable-value-field/lockable-value-field.tsx:387
- MultiSelect is now rendered inside an InputGroup, but its internal trigger button isn’t marked with
data-slot="input-group-control", so the InputGroup wrapper can’t apply the unified focus ring/disabled styling (input-group.tsx:47/52). This can leave the right-side addons outside the visible focus indicator during keyboard navigation.
<MultiSelect
id={fieldId}
className="min-w-0 flex-1"
options={options}
selected={parseListValue(value)}
onChange={(selected) => onValueChange?.(JSON.stringify(selected))}
placeholder="Select options..."
disabled={!onValueChange}
onBlur={onValueBlur}
aria-invalid={error ? true : undefined}
aria-describedby={error ? validationId : undefined}
aria-errormessage={error ? validationId : undefined}
/>
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are additive, well-covered by updated tests/stories, and align the UI patterns consistently without introducing verified functional or API regressions.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are internally consistent, covered by targeted tests for the new LockableValueField behaviors, and the Storybook-only deep-linking additions are scoped to client-side effects.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
b7329e6 to
455f130
Compare
Review scope
This PR is organized by component and pattern so each change can be reviewed independently:
Input
Dropdown
Combobox
Lockable Value Field
File Upload
Date Picker and DateTime Picker
Button
UI Inventory
Flow theme regression
Verification
git diff --checkpassed.