Add LockableValueField component and HITL panel#960
Open
dbacomputer wants to merge 15 commits into
Open
Conversation
Bring the Form HITL / Form HITL Review NodePropertyPanel stories over from the hitl-prototype branch as the starting point for packaging this work up for review: a quick-form builder panel (drag-reorder fields, per-field lock/read-only, Fixed value vs. JS expression, AI generate, JSON schema view) built on the new LockableValueField. This lands the prototype's current state as-is; the next two commits remove the now-redundant Review page and switch consumption over to LockableValueField from @uipath/apollo-wind. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move LockableValueField into apollo-wind as a real, consumable component (Components/Core, alongside Input Group) instead of a one-off file living inside apollo-react's canvas panel. Its import surface was already 100% apollo-wind + lucide-react, so this is a straight port, not a rewrite. - packages/apollo-wind/src/components/ui/lockable-value-field.tsx - lockable-value-field.stories.tsx (Default / Controls / Responsive) - lockable-value-field.test.tsx (new coverage; it had none before) - src/index.ts exports, following the same pattern as InputGroup - storySort order updated so it sits right after Input Group Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Drop FormHitlReviewStory, its FormHitlReview export, the
REVIEW_LOCKABLE_CASES fixture, and the ApproveRejectActions header --
Form HITL is the one page going forward, no need for two.
The showFieldActions={false} reviewer-lockdown behavior this page
demonstrated isn't lost: it's still a real prop on LockableValueField,
just without a dedicated story exercising it after this cleanup.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Import LockableValueField (and FIELD_TYPE_META, LockableFieldType,
LockableValueFieldMode) from @uipath/apollo-wind instead of the local
file, and delete the now-redundant local copy -- apollo-wind is the
source of truth going forward.
Also relabels the right-side demo panel from "LockableValueField" to
"Demo controls" and drops its description/expandable-details copy,
since that content now duplicates the real component's docs page. The
panel links out to that page instead ("Uses component -> Lockable
Value Field"), making clear this panel is a control surface for the
demo (its Show/Hide toggle drives controlsVisibility on the left
panel's fields too) rather than documentation to read.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions
Bot
added
pkg:apollo-react
app:storybook
pkg:apollo-wind
size:XXL
Contributor
Dependency License Review
License distribution
Excluded packages
|
There was a problem hiding this comment.
Pull request overview
Promotes LockableValueField from a one-off prototype into a first-class apollo-wind UI component (with Storybook docs + unit tests), and updates the apollo-react NodePropertyPanel stories to consume it from @uipath/apollo-wind while placing it next to other core inputs in Storybook navigation.
Changes:
- Added
LockableValueFieldcomponent implementation in apollo-wind, plus Storybook stories and Vitest/jest-axe coverage. - Exported the component (and related types/constants) from
packages/apollo-wind/src/index.ts. - Updated
NodePropertyPanel.stories.tsxand StorybookstorySortso the component is discoverable and the prototype panel consumes the shared implementation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/apollo-wind/src/index.ts | Exposes LockableValueField (and related exports) from the apollo-wind public entrypoint. |
| packages/apollo-wind/src/components/ui/lockable-value-field.tsx | New component implementation (lock/unlock, fixed vs expression mode, multiple field types, header actions). |
| packages/apollo-wind/src/components/ui/lockable-value-field.test.tsx | Adds unit + accessibility tests for key behaviors and gating props. |
| packages/apollo-wind/src/components/ui/lockable-value-field.stories.tsx | Adds Storybook docs + demos (Default/Controls/Responsive) showing intended usage patterns. |
| packages/apollo-react/src/canvas/components/NodePropertyPanel/NodePropertyPanel.stories.tsx | Updates/extends prototype story to import and use LockableValueField from apollo-wind. |
| apps/storybook/.storybook/preview.tsx | Adjusts Storybook sort order to place “Lockable Value Field” near Input/Input Group under Core components. |
Comments suppressed due to low confidence (1)
packages/apollo-wind/src/components/ui/lockable-value-field.tsx:534
- The unlocked date path formats with
new Date(value).toLocaleDateString(...)and passesnew Date(value)to the calendarselectedprop without validating the date. Invalid date strings can throw (RangeError) or break the calendar. Guard invalid dates and fall back to showing the raw value while leaving the calendar unselected.
{value ? (
new Date(value).toLocaleDateString(undefined, {
year: 'numeric',
month: 'long',
day: 'numeric',
Contributor
📊 Coverage + size by packagePer-package coverage and bundle size on this PR. New-line coverage = of the source lines this PR adds or changes, the % hit by tests.
"Coverage" is each package's own |
- Stop exporting DEMO_SELECT_OPTIONS and parseListValue from the package root -- they're internal implementation details for the single/multi-select demo options, not stable public API. - Fix the default label to key off effectiveMode instead of raw mode, so it can't say "Write a string expression" while a fixed-value control is rendered for a field type that doesn't support expressions. - Guard date parsing/formatting against invalid date strings: a malformed value prop no longer throws a RangeError when locked, or gets passed to the calendar as an Invalid Date when unlocked -- both now fall back cleanly (raw value / unselected). Added regression tests for both paths. - Validate parsed JSON items in the Form HITL quick-form's schema editor before assigning them as cases, so a malformed hand-edit (missing id, unknown fieldType, etc.) surfaces as a JSON error instead of breaking Sortable or rendering. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…resolution
Two pre-existing repo issues were surfacing as Format/Lint failures on
this PR (confirmed unrelated to its actual diff -- both reproduce on
main):
- packages/apollo-core, apollo-react, and apollo-wind's biome.json
were all missing "root": false, so Biome 2.x treated each as its own
conflicting root configuration instead of a child of the repo-root
config ("Found a nested root configuration, but there's already a
root configuration"). apps/apollo-vertex/biome.json already had this
set correctly; the other three didn't.
- pr-checks.yml's `pnpm lint/format:check --affected` relies on
Turborepo's own GitHub Actions event auto-detection to find the PR's
base ref, which isn't resolving reliably here -- it was silently
falling back to treating every package in the monorepo as affected,
instead of just the ones a given PR actually touches. Setting
TURBO_SCM_BASE explicitly to the base commit SHA sidesteps the
auto-detection: verified locally that --affected correctly narrows
to just the changed packages (+ dependents) once it's set, versus
all 9 packages without it.
Also removes one now-genuinely-unused biome-ignore suppression comment
in panel-flow.tsx (unrelated file, surfaced once the config fix let
Biome actually run) -- biome itself flagged it as ineffective and
recommended removing it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dbacomputer
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (3)
packages/apollo-wind/src/components/ui/lockable-value-field.tsx:547
- The calendar stores the selected date via
toISOString(), which can shift the day depending on timezone (local midnight converted to UTC). For a date-only field, store a date-only string derived from local year/month/day (or otherwise use a timezone-stable encoding).
<Calendar
mode="single"
selected={parseDateValue(value)}
onSelect={(date) => onValueChange?.(date ? date.toISOString() : '')}
initialFocus
packages/apollo-wind/src/components/ui/lockable-value-field.tsx:629
- Same as above: in the non-expression branch, the lock toggle should be disabled (and its aria-label shouldn’t suggest clicking will toggle) when
onLockedChangeisn’t provided.
<InputGroupButton
icon
size="3xs"
onClick={() => onLockedChange?.(!locked)}
aria-label={
locked
packages/apollo-wind/src/components/ui/lockable-value-field.tsx:247
- The default label/placeholder text is always “String value” / “Write a string expression”, even when
fieldTypeisinteger,date, etc. That’s misleading for consumers who don’t pass a customlabel. Consider generating these strings fromfieldTypemetadata (and updating the related tests that assert the placeholder).
const FIELD_LABEL: Record<LockableValueFieldMode, string> = {
fixed: 'String value',
expression: 'Write a string expression',
};
const FIELD_PLACEHOLDER: Record<LockableValueFieldMode, string> = {
fixed: 'String value',
expression: 'Write a string expression',
};
- Date parsing/formatting now treats date-only strings (YYYY-MM-DD) as a local date instead of routing them through `new Date(string)`, which reads them as UTC midnight and can display the wrong day in negative-UTC-offset timezones. Selecting a date now stores it back as a plain local YYYY-MM-DD string too, for a fully round-trippable, timezone-safe fixed-date flow. - The lock toggle is now disabled (with an adjusted, non-actionable aria-label) when onLockedChange isn't provided, instead of looking clickable while doing nothing. Extracted a shared LockToggleButton component so this -- and any future fix -- only needs to happen once, not twice (the previous round's date bug had already shown up in two copy-pasted places). - Added an `options` prop for single-select / multi-select, defaulting to the existing demo set, so those field types are actually consumable outside of demos. - Added an `onGenerateWithAi` callback, called with the entered prompt when Generate is clicked; the button is disabled when it isn't provided, matching the same "don't look interactive if it wouldn't do anything" fix applied to the lock toggle. - The default label now always has a real `id` to associate with (falls back to a generated one via useId when the id prop isn't set), fixing a case where the Label had nothing to point at. 8 new tests covering all five; full suite still green (1024 tests). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Apollo Coded App preview deployments are ready.
|
- Show an empty locked display instead of "False" for an unset boolean value, since only "true"/"false" are valid. - Disable the Fixed/Expression dropdown trigger when onModeChange isn't provided, matching the pattern already used for the lock toggle. - Reflect the current field type (and expression support) in the AI-assist popover's output hint instead of hardcoding "String expression". - Use an own-property check in isValidLockableCase so inherited properties (e.g. "toString") can't be mistaken for a valid fieldType.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
packages/apollo-wind/src/components/ui/lockable-value-field.tsx:315
- The default label/placeholder are hard-coded to "String value" / "Write a string expression", but the component supports non-string
fieldTypes (integer/date/boolean). If a consumer setsfieldTypewithout also providing a customlabel, the UI will be misleading (e.g. showing "String value" for an Integer). Consider deriving the default label/placeholder fromfieldType(andeffectiveMode) usingtypeMeta.labelso they stay accurate across types.
const FIELD_LABEL: Record<LockableValueFieldMode, string> = {
fixed: 'String value',
expression: 'Write a string expression',
};
const FIELD_PLACEHOLDER: Record<LockableValueFieldMode, string> = {
fixed: 'String value',
expression: 'Write a string expression',
};
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
packages/apollo-wind/src/components/ui/lockable-value-field/utils.ts:78
- For locked multi-select fields, invalid/malformed stored values currently parse to an empty list (via
parseListValue) and render as blank, even though a non-empty raw value exists. Consider treating malformed/non-array JSON as “unknown” and falling back to the raw string so the locked display never silently drops data.
case 'multi-select':
return parseListValue(value)
.map((v) => options.find((option) => option.value === v)?.label ?? v)
.join(', ');
BenGSchulz
reviewed
Jul 24, 2026
| }, | ||
| file: { | ||
| label: 'File', | ||
| icon: Paperclip, |
Contributor
There was a problem hiding this comment.
We have been using File as the icon for file-type fields in flow-workbench. If this is an explicit decision to change to Paperclip we should align all other usage too.
Collaborator
Author
There was a problem hiding this comment.
Good catch, thank you. This was an unreviewed choice on my part, not an intentional divergence from flow-workbench's File icon. Leaving this open rather than resolving it. I want to revisit icon consistency with the HITL team after this PR merges, since they're the ones driving these decisions going forward for their surface.
BenGSchulz
reviewed
Jul 24, 2026
BenGSchulz
left a comment
BenGSchulz
left a comment
Contributor
There was a problem hiding this comment.
IMO we should still consider renaming the component / folder because there is a lot more functionality here than the lockable aspect of the field. This seems like a full inline field builder/designer (field-builder-inline?)
… stories The stories file moved into the lockable-value-field/ folder during the prior split, but its relative imports weren't updated: `./label` and `./toggle-group` needed an extra `../` for the new nesting depth, and FIELD_TYPE_META/LockableFieldType/LockableValueFieldMode moved to types.ts and are no longer exported from the main component file. .stories.tsx files are excluded from the tsconfig, so tsc never caught this -- it only surfaced as a generic Vite "failed to fetch dynamically imported module" error in the browser.
- Fall back to the raw value for a locked single-select display when it isn't present in options, instead of hiding it as an empty string (e.g. after options change out from under a stored value). - Compute the expected locale date string via the same toLocaleDateString call instead of hardcoding an English date, so the timezone test doesn't depend on the CI/dev machine's locale.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
packages/apollo-wind/src/components/ui/lockable-value-field/lockable-value-field.tsx:245
- Similarly for the file field: the visible field label isn’t associated with the FileUpload control, whose accessible name defaults to generic “File upload area/File upload”. Consider adding id/aria-labelledby/aria-label support to FileUpload and wiring it from LockableValueField so the accessible name matches the actual field label.
fieldType === 'file' && (
<FileUpload
className="flex-1"
onFilesChange={(files) => onValueChange?.(files.map((f) => f.name).join(', '))}
/>
)
- parseListValue now filters to strings, so a malformed stored value (e.g. numbers/null slipping into the JSON array) can't leak past the string[] contract into MultiSelect or the locked display. - Disable Insert-variable when variables are supplied but onValueChange isn't, instead of rendering a selectable menu that silently no-ops. - Give multi-select and file fields a real accessible name: MultiSelect now accepts an id (wired to the trigger button, associates with a <label htmlFor> the same way Select already does), and FileUpload accepts an ariaLabel override for its non-labelable role="group" dropzone, both driven by the field's computed label. - Use item.title instead of the array index as the React key in panel-flow.tsx's tool-item list.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
packages/apollo-wind/src/components/ui/lockable-value-field/lockable-value-field.stories.tsx:244
- The
labelconstant here is a<label>withouthtmlFor, and it’s reused across multipleLockableValueFieldinstances. That means none of the rendered controls get a proper accessible name from the label, and addinghtmlForrequires unique ids per instance to avoid duplicate ids in the DOM.
const label = (
<Label className="text-xs font-medium text-foreground-muted">
Label
{required && <span className="ml-0.5 text-destructive">*</span>}
</Label>
);
packages/apollo-react/src/canvas/components/NodePropertyPanel/NodePropertyPanel.stories.tsx:2964
- Same issue in the compact demo:
<Label>is missinghtmlFor, so assistive tech won’t associate the label text with the field control. Pair a uniqueidwithLabel htmlForhere too.
<LockableValueField
label={
<Label className="text-xs font-medium text-foreground-muted">
Label
{showcaseRequired && <span className="ml-0.5 text-destructive">*</span>}
</Label>
}
headerActions={
- parseDateValue now rejects date-only strings whose components are out of range (e.g. 2024-13-40) instead of letting new Date() silently normalize them into a different valid date. - Use a composite icon+title+description key for panel-flow.tsx's tool-item list instead of item.title alone, since title isn't guaranteed unique. - Wire id + Label htmlFor together in the LockableValueField stories (apollo-wind's Default/Responsive demos and apollo-react's NodePropertyPanel showcase) so the demoed label is actually associated with its field, instead of rendering a real <label> with no htmlFor.
4 tasks
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
packages/apollo-wind/src/components/ui/multi-select.tsx:79
idwas added so a<label htmlFor>can associate with the trigger button, but the trigger still sets anaria-label.aria-labeloverrides the associated<label>in the accessible-name computation, so screen readers will announce the placeholder / selection count instead of the visible label.
Consider omitting aria-label when id is provided (so the external <label> can be used), and keeping the current fallback when id is not provided.
id={id}
variant="outline"
role="combobox"
aria-expanded={open}
aria-label={selected.length > 0 ? `${selected.length} items selected` : placeholder}
Resolves conflicts in NodePropertyPanel.stories.tsx: - Keeps our LockableValueField/Form HITL additions and the pre-existing inline Output-panel prototype (OutputNode/TypeBadge/Concept2PanelStory/ InputOutputStory), rather than adopting main's coworker commit that replaced that prototype with the new NodeIOView component -- that migration is unrelated to this PR's scope and can happen separately. - Restores ChevronsDownUp/ChevronsUpDown/CircleDot/CircleOff to the lucide-react import list: git's non-conflicting auto-merge silently dropped them from main's side even though our kept code still uses them, since main's edit and ours never touched the same line. - Rebuilds apollo-wind's dist so apollo-react's typecheck picks up MetadataForm's new sectionVariant prop, added by main.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packages/apollo-wind/src/components/custom/panel-flow.tsx:228
keyfor the mappedcard.itemslist is derived from icon/title/description, butFlowPanelToolItemdoesn't guarantee those are unique. If two items share the same values, React will see duplicate keys and may render/update rows incorrectly. Include an index suffix (or add a stableidfield toFlowPanelToolItem) to guarantee uniqueness.
{card.items.map((item) => {
const Icon = toolIconMap[item.icon];
return (
<div
key={`${item.icon}-${item.title}-${item.description}`}
className="flex items-start gap-2.5 pl-4 pr-9"
>
"Multiselect" was inconsistent with the single-select label ("Single
select") and with the component's own Storybook title ("Multi Select"
in multi-select.stories.tsx). Updates the story doc copy and test
assertion to match.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
packages/apollo-wind/src/components/custom/panel-flow.tsx:226
- The new key
${item.icon}-${item.title}-${item.description}can collide if the card contains duplicate items (same icon/title/description), which will cause unstable rendering and React key warnings. Including the index (or adding a stableidfield toFlowPanelToolItem) avoids collisions.
{card.items.map((item) => {
const Icon = toolIconMap[item.icon];
return (
<div
key={`${item.icon}-${item.title}-${item.description}`}
Comment on lines
+83
to
+86
| case 'multi-select': | ||
| return parseListValue(value) | ||
| .map((v) => options.find((option) => option.value === v)?.label ?? v) | ||
| .join(', '); |
Comment on lines
74
to
76
| <Button | ||
| id={id} | ||
| variant="outline" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Promotes
LockableValueFieldfrom a one-off prototype file into a real, documented, tested component in apollo-wind, and updates the apollo-react Form HITL panel to consume it.Apollo Wind — what to review
A new component, so review it fresh:
components/ui/lockable-value-field.tsx): a field that locks to read-only, supports several data types, and (for scalar types) toggles between a literal value and a JS expression.src/index.ts):LockableValueField, its types, andFIELD_TYPE_META/FIELD_TYPE_ORDERare exported;DEMO_SELECT_OPTIONS/parseListValueintentionally are not (internal demo-data helpers, not stable API) — worth confirming that's the right surface to commit to.Default/Controls/Responsivepages underComponents → Core → Lockable Value Field, placed next toInput/Input Group.modeinstead ofeffectiveMode(could say "Write a string expression" while showing a fixed-value control); date parsing/formatting now guards against invalid date strings instead of throwing or handing the calendar an Invalid Date.Apollo React — what to review
An existing prototype panel being updated, not new code to review line-by-line — the main things worth confirming:
NodePropertyPanel.stories.tsxnow importsLockableValueFieldfrom@uipath/apollo-windinstead of a local file (deleted). Worth a quick check that nothing behaves differently.showFieldActions={false}) is still a real prop, just without its own story. Flag if you'd rather keep a dedicated example of that.Also fixed (repo CI, unrelated to this PR's actual feature work)
Format/Lint were failing on this PR for pre-existing repo reasons, both reproduce on
main:apollo-core,apollo-react, andapollo-wind'sbiome.jsonwere all missing"root": false, so Biome 2.x rejected them as conflicting root configs.pr-checks.yml's--affectedflag wasn't reliably resolving the PR base ref, silently falling back to checking every package in the monorepo. Now setsTURBO_SCM_BASEexplicitly.Test plan
tsc --noEmitclean on both packagesvitestsuites green (apollo-wind 1016/1016, apollo-react 2070/2070)biome lint/biome formatclean (exit 0) on apollo-wind, apollo-react, and apollo-core, matching each package's actual CI scriptcontrolsVisibilitytoggle, and the "Uses component →" doc linkCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com