docs(apollo-wind): add Field Types reference page - #1143
dbacomputer wants to merge 16 commits into
Conversation
Cross-references every field/property/parameter type flow-workbench needs (across workflow variables, JSON Schema manifests, entity fields, HITL forms, and the integration-service widget catalog) against what LockableValueField and Apollo Wind support today. Each row gets a live LockableValueField example where the type is supported, a "no control yet" placeholder where it is not, and a status (Supported / Needs fieldType / Needs component) with a recommended next step, so engineers have one place to check before adding a new type case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Apollo Coded App preview deployments are ready.
|
Expression-capable types (string, integer, date, boolean, file, object) now show both the fixed and expression visual examples stacked in the same cell, since the mode toggle changes the control's placeholder and styling, not just its value. Types that don't support expressions (single-select, multi-select) keep a single example, matching FIELD_TYPE_META.supportsExpression. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Dependency License Review
License distribution
Excluded packages
|
📊 Coverage + size by packagePer-package bundle size on this PR (no JS/TS source changes detected under
"Coverage" is each package's own |
There was a problem hiding this comment.
🟡 Changes recommended
Binding and lock interactions are disabled, and the integer example does not enforce integer-only input.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a Storybook reference page documenting field-type support in Apollo Wind.
Changes:
- Adds categorized field-type support tables and status guidance.
- Provides live examples and placeholders for unsupported controls.
- Documents binding states and recommended follow-up actions.
File summaries
| File | Summary |
|---|---|
apps/storybook/src/patterns/FieldTypes.stories.tsx |
New Field Types reference page with interactive examples. |
Review details
Suppressed comments (1)
apps/storybook/src/patterns/FieldTypes.stories.tsx:481
- The default lock control is still rendered, but no
onLockedChangeis supplied, soLockToggleButtonintentionally disables it. These examples therefore cannot exercise the lock/read-only transition even though the page describes them as live interactive fields; keep local locked state and passonLockedChange(or explicitly hide the lock control if that interaction is out of scope).
const [mode, setMode] = useState<LockableValueFieldMode>(initialMode);
return (
<div className="w-44">
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| { | ||
| type: 'number / float / double', | ||
| source: 'hitl-schema-types (float, double), JSON Schema number', | ||
| support: 'Renders via integer today, but the control does not allow decimals.', |
There was a problem hiding this comment.
🔵 Needs a closer look
Address the two moderate issues and status-label ambiguity before approval.
Review details
Suppressed comments (3)
apps/storybook/src/patterns/FieldTypes.stories.tsx:99
- This row says the integer control does not allow decimals, but
LockableValueFieldonly renderstype="number"and forwards the entered string without integer validation, so a value such as1.5can still be emitted. That makes this audit inaccurate; either enforce integer input in the component or describe the current lack of decimal-specific semantics/validation.
support: 'Renders via integer today, but the control does not allow decimals.',
apps/storybook/src/patterns/FieldTypes.stories.tsx:514
ModeExampledoes not passonModeChange, soLockableValueFieldrenders its fixed/expression menu disabled. The binding-state examples therefore cannot switch modes, despite the page's live/interactive claim and test plan. Keep local mode state and passonModeChangeasFieldTypeExampledoes.
function ModeExample({ mode }: { mode: LockableValueFieldMode }) {
const [value, setValue] = useState(mode === 'expression' ? '$vars.example' : 'Example value');
return (
<div className="w-44">
<LockableValueField
fieldType="string"
value={value}
onValueChange={setValue}
locked={false}
mode={mode}
showFieldActions={false}
/>
apps/storybook/src/patterns/FieldTypes.stories.tsx:109
- This row is labeled
Needs fieldType, but its support text says the existing integer control already renders it and the recommendation explicitly allows no UI change. That conflicts with the status legend, whereNeeds fieldTypemeans a new component field type or format is needed, and makes the reference ambiguous for consumers deciding whether to add a case. If the 32/64-bit distinction is backend-only, mark this as supported and document that constraint; otherwise describe the concrete UI field type that is required.
type: 'int32 / int64',
source: 'hitl-schema-types schema definitions',
support: 'Renders via integer.',
status: 'needs-type',
action:
'Confirm whether the 32 vs. 64 bit distinction matters at the UI layer. If it is a backend validation constraint only, no UI change is needed.',
- Files reviewed: 1/1 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 09, 2026, 07:14:24 PM |
Recommended action now reads as the second-to-last column instead of Source in flow-workbench, since the action is the thing engineers act on and the source citation is reference material for double-checking. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved findings affect interactive examples, table accessibility, and documented field behavior.
Review details
Suppressed comments (7)
apps/storybook/src/patterns/FieldTypes.stories.tsx:487
- This example renders the lock toggle with
locked={false}but omitsonLockedChange, so the component disables the visible control. Because this page presents the controls as live interactive examples, clicking the lock is a no-op; hide the lock for this fixed/expression-only demo or add local locked state and a callback.
mode={mode}
apps/storybook/src/patterns/FieldTypes.stories.tsx:511
- This mode example has the same non-functional lock affordance:
LockableValueFielddisables its lock button whenonLockedChangeis absent. Either wire the example to locked state or hide the lock so the interactive demo does not expose a control that cannot be used.
locked={false}
apps/storybook/src/patterns/FieldTypes.stories.tsx:557
- Each category is rendered as an HTML table, but the visible section heading is not associated with it and the table has no caption or accessible name. Screen-reader users will encounter a series of anonymous tables and cannot tell which category they represent; add a caption or label the table with its
title.
<Table>
apps/storybook/src/patterns/FieldTypes.stories.tsx:99
- This says the integer control does not allow decimals, but
LockableValueFieldrenders it as an<input type="number">without a decimal-rejectingstepor validation and forwards typed strings throughonValueChange. Reword this as the absence of decimal-specific semantics/validation so the reference does not promise behavior the component does not provide.
support: 'Renders via integer today, but the control does not allow decimals.',
apps/storybook/src/patterns/FieldTypes.stories.tsx:219
- The status legend defines
needs-typeas a case that fits the existing single-value model, but this row requires add/remove/reorder behavior for arbitrary sub-types. That is a dedicated multi-value editor like the dictionary/collection rows, so this status currently recommends extending a component that cannot represent the interaction model; classify it asneeds-componentinstead.
status: 'needs-type',
action:
'Add a generic array fieldType: add, remove, and reorder rows of an arbitrary sub-type.',
apps/storybook/src/patterns/FieldTypes.stories.tsx:152
- Masking is a security treatment for secrets, not a general affordance for email, URI, or UUID values; applying it here would hide ordinary identifiers without improving format handling. Recommend format-specific validation/input behavior instead, and reserve masking for the Security rows.
'Add format-level validation and masking. Likely a format prop on string rather than a new fieldType.',
apps/storybook/src/patterns/FieldTypes.stories.tsx:109
- This row is labeled
Needs fieldType, while its support text says it already renders throughintegerand its action explicitly allows that no UI change is needed when the distinction is backend-only. That makes the status column ambiguous for this reference; choose eithersupportedfor the UI layer or a clearly required UI distinction.
status: 'needs-type',
action:
'Confirm whether the 32 vs. 64 bit distinction matters at the UI layer. If it is a backend validation constraint only, no UI change is needed.',
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
…workbench confidence flag - Each category (and Binding state) is now an independently collapsible accordion item, with a live gap count in its trigger, so a reviewer can expand just the sections they care about instead of scrolling past all twelve. - Table header is smaller, denser, and visually distinct from body rows (uppercase, shorter cells, tinted background) instead of matching the Table primitive's default prose-sized header. - Added flowWorkbenchConfirmed to TypeRow and marked the 8 rows where flow-workbench itself may not treat the type as distinct (int32/int64, verbatim, collection, the stringArray family, checkboxGroup, radioGroup, null, ref) with a dagger and an explicit legend callout. Status previously only described the Apollo Wind side; for these rows the real gap may be that no one has decided the distinction is real yet, in either place, not that Apollo Wind is missing something confirmed to exist. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Five unresolved findings remain, including four moderate issues.
Review details
Suppressed comments (5)
apps/storybook/src/patterns/FieldTypes.stories.tsx:524
LockToggleButtondisables itself whenonLockedChangeis omitted, so every field-type example shows a visible lock control that cannot be used. Since this page presents these as live interactiveLockableValueFieldexamples, either wire a local locked state or hide the lock control so the demo does not advertise a non-functional affordance.
locked={false}
apps/storybook/src/patterns/FieldTypes.stories.tsx:551
- This fixed/expression example also omits both
onLockedChangeandonModeChange. The rendered lock and value-mode controls are therefore disabled byLockableValueField, so the binding-state examples are not interactive even though they are presented as live examples. Add local handlers or hide the controls when this is intentionally a static comparison.
locked={false}
mode={mode}
showFieldActions={false}
apps/storybook/src/patterns/FieldTypes.stories.tsx:619
- The explanation for the † marker is available only through
titleon a non-focusable<sup>. Keyboard users and assistive technologies cannot reliably discover why a row is marked, even though the legend says this distinction affects how the gap should be interpreted. Use the existing accessible tooltip pattern or an associated visible/ARIA description and make the marker keyboard discoverable.
<sup
className="ml-0.5 cursor-help text-muted-foreground"
title="Not confirmed as a distinct, separately-rendered type in flow-workbench either, only a schema-level type name. See Recommended action."
apps/storybook/src/patterns/FieldTypes.stories.tsx:108
- The current integer branch renders an
<input type="number">without astepconstraint or integer validation, so decimal strings can still be entered and passed toonValueChange. Saying the control "does not allow decimals" makes this reference inaccurate; describe the limitation as missing decimal-specific semantics/validation instead.
support: 'Renders via integer today, but the control does not allow decimals.',
apps/storybook/src/patterns/FieldTypes.stories.tsx:119
- This row is classified as
Needs fieldType, whose legend says a new fieldType is required, even though the support text says it already renders viaintegerand the recommended action explicitly says no UI change may be needed. That status will direct engineers toward adding unnecessary variants; either classify it as supported viaintegeror introduce a separate unresolved-source-type status.
type: 'int32 / int64',
source: 'hitl-schema-types schema definitions',
support: 'Renders via integer.',
status: 'needs-type',
action:
'Confirm whether the 32 vs. 64 bit distinction matters at the UI layer. If it is a backend validation constraint only, no UI change is needed.',
flowWorkbenchConfirmed: false,
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
The live LockableValueField demos were 176px wide, too narrow for their own placeholder text (e.g. "Integer value" got clipped to "Integer vali" with no ellipsis), which read as a layout bug rather than the intentional live-preview it was. Widened the demo containers and gap placeholder to 224px and rebalanced column widths to make room. Also made the accordion open state controlled and added Expand all / Collapse all buttons above the category list, since toggling twelve sections one at a time to get back to "everything open" was tedious. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- "Source in flow-workbench" -> "Source" (redundant given the page title and intro already establish the flow-workbench context). - The status-legend callout had no top margin, so it sat flush against the 3-card grid above it. Also dropped its bordered "card" styling: a fourth, differently-shaped box competing with a 3-card grid read as a mismatched extra card rather than explanatory text, so it's now a plain paragraph with proper spacing instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Address the unresolved interactive-demo and integer-behavior documentation issues.
Review details
Suppressed comments (4)
apps/storybook/src/patterns/FieldTypes.stories.tsx:552
- The binding-state examples are not actually interactive: because this instance omits
onModeChange,LockableValueFielddisables its value-mode menu (disabled={!onModeChange}). The page therefore shows fixed/expression snapshots but users cannot switch between them, contrary to the live-demo intent. Keep the initial mode in local state and passonModeChange={setMode}.
showFieldActions={false}
apps/storybook/src/patterns/FieldTypes.stories.tsx:109
- This support description is inaccurate for the current implementation:
LockableValueFieldrendersintegeras an<input type="number">and forwards every entered string throughonValueChange, without integer validation, so a user can type and propagate a decimal such as1.5. Please describe the current behavior as an integer-oriented numeric input with no decimal-specific type/validation, or add actual validation before documenting decimals as disallowed.
support: 'Renders via integer today, but the control does not allow decimals.',
apps/storybook/src/patterns/FieldTypes.stories.tsx:636
- This standalone em dash is user-facing Storybook copy, which violates the repository rule against em dashes in story documentation. Use a text fallback such as
Not applicablefor supported rows instead.
{row.action ?? '—'}
apps/storybook/src/patterns/FieldTypes.stories.tsx:435
- Expression is not supported for every field type:
FIELD_TYPE_METAmarkssingle-selectandmulti-selectwithsupportsExpression: false, andLockableValueFieldforces those types back to fixed mode. Marking the binding state as universally Supported can mislead readers into assuming this works for choice fields; qualify the row by supported field types or split the status.
type: 'expression (LockableValueFieldMode) ↔ Expression (integration-service ValueType)',
source: 'lockable-value-field/types.ts, integration-service ValueType',
support: 'mode="expression"',
status: 'supported',
visual: 'mode-expression',
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Add a "What each type looks like" section right after "Supported today": all eight supported fieldTypes side by side, each in its default fixed-value state, reusing the same live FieldTypeExample the tables below use. Distinct job from the per-category tables: a quick visual comparison across types, versus drilling into one type's gaps and status. Gave the new section its own <Divider /> and <SectionTitle>, matching "Supported today" in both structure and heading size instead of an ad-hoc smaller heading. FileUpload's dropzone is a fixed h-32 with its own "Click to upload or drag and drop" copy, sized for a full-width form field. That blew up the grid's row height and visually overlapped the section below it. Added an optional className to FieldTypeExample and a targeted descendant-selector override that collapses the File cell to a single icon-only row matching the other seven, only in this compact grid. The dropzone's accessible name (aria-label="File upload area") doesn't depend on the now-hidden paragraph text, so this doesn't regress accessibility. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
New section between "What each type looks like" and "Supported
today": four live examples covering cross-cutting states that sit on
top of fieldType, not documented anywhere on this page before:
- Locked (read-only display)
- Unlocked, lock shown (default)
- Unlocked, lock hidden (showLock={false}) - had no story anywhere,
not here, not in lockable-value-field.stories.tsx itself
- Assignment binding (leadingAddon="=", the node-property pattern
from that file's own "Assignment & Binding" reference example)
Deliberately does not recreate Input's or Input Group's own state
variants here (Components/Core/Input has 11, Input Group has 8,
including a LockedFieldWithPopover recipe already covering similar
ground) - a callout points to those pages instead of duplicating
content that already has a more complete, canonical home.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Resolve the identified interaction, responsive-layout, accessibility, and support-text issues.
Review details
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
apps/storybook/src/patterns/FieldTypes.stories.tsx:670
- The dagger's explanation is available only through a
titleon a non-focusable<sup>, which is not reliably exposed to keyboard or screen-reader users. Because this marker carries the row's confidence status, add an accessible/visually-hidden explanation (or another keyboard-accessible disclosure) instead of relying on the hover title.
apps/storybook/src/patterns/FieldTypes.stories.tsx:581
ModeExamplenever passesonModeChange, soLockableValueFieldrenders its Fixed/Expression trigger disabled (disabled={!onModeChange}). The two binding-state examples therefore cannot perform the promised interactive mode toggle; keep local mode state and pass it throughonModeChangeasFieldTypeExampledoes.
function ModeExample({ mode }: { mode: LockableValueFieldMode }) {
const [value, setValue] = useState(mode === 'expression' ? '$vars.example' : 'Example value');
return (
<div className="w-56">
<LockableValueField
fieldType="string"
value={value}
onValueChange={setValue}
locked={false}
mode={mode}
showFieldActions={false}
/>
apps/storybook/src/patterns/FieldTypes.stories.tsx:723
- On narrow viewports the grid remains two columns, while every example is fixed at
w-56(224px). With the page'sp-8padding, two examples plus the gap cannot fit, so the documentation page overflows horizontally instead of reflowing. Use a single column below the breakpoint (or make the examples fluid) before switching to two/four columns.
<div className="grid grid-cols-2 gap-4 sm:grid-cols-4">
{FIELD_TYPE_ORDER.map((type) => {
apps/storybook/src/patterns/FieldTypes.stories.tsx:701
- The page text says the supported strip is read live from
FIELD_TYPE_META, but both this strip and the visual grid iterate the separateFIELD_TYPE_ORDERarray. Those two exports can diverge when a new field type is added, leaving the page out of sync while still appearing valid. Make one shared source authoritative for both the component and this page, or change the documentation to identifyFIELD_TYPE_ORDERas the source of truth.
{FIELD_TYPE_ORDER.map((type) => {
const typeMeta = FIELD_TYPE_META[type];
apps/storybook/src/patterns/FieldTypes.stories.tsx:111
- This row says the integer control does not allow decimals, but
LockableValueFieldrenders it as an<input type="number">and forwards the entered string without integer validation or astepconstraint, so decimal text can still be entered. Either enforce integer values in the component or reword this support text to describe the missing decimal-specific type/validation.
support: 'Renders via integer today, but the control does not allow decimals.',
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Two changes based on feedback: - Reformatted the LockableValueField states section from a 2x2 card grid to a table (State, Example, Notes columns), reading left to right like every other table on this page, via a new shared StatesTable component. - Added Input and Input Group sections above "What each type looks like," each its own StatesTable (Default, Disabled, Invalid, plus Read-only for Input and a Locked read-only recipe for Input Group). This page is meant to document every field-level building block used on a form, not just LockableValueField, so these get shown directly rather than just linked to from a callout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Responsive layout, example interactivity, and filtered gap counts need correction.
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.
apps/storybook/src/patterns/FieldTypes.stories.tsx:1047
- When
onlySupportedis enabled,visibleSectionscontains only supported rows, so this count is calculated from the filtered rows. Categories such as Temporal and Binding state therefore display “all supported” even though their unsupported rows are merely hidden by the filter, which makes the promised live gap count inaccurate. Compute the count from the corresponding unfilteredALL_SECTIONSentry while usingsection.rowsonly for the table contents.
apps/storybook/src/patterns/FieldTypes.stories.tsx:540
- The comparison grid switches to two columns below
sm, but everyFieldTypeExampleis fixed atw-56(224px). On a phone the two grid tracks are much narrower than each live control, so the examples overflow the page instead of fitting the responsive layout. Let this wrapper shrink within its track while retaining the desktop cap (for example,w-full max-w-56 min-w-0).
}: {
apps/storybook/src/patterns/FieldTypes.stories.tsx:580
LockableValueFielddisables the Fixed/Expression trigger whenonModeChangeis omitted (seelockable-value-field.tsx:294-299). These binding-state examples therefore show a disabled mode control and cannot exercise the toggle, contrary to the page's interactive examples; keep the mode in local state and passonModeChangehere.
</span>
);
}
function ModeExample({ mode }: { mode: LockableValueFieldMode }) {
const [value, setValue] = useState(mode === 'expression' ? '$vars.example' : 'Example value');
return (
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
| function UnlockedLockShownExample() { | ||
| const [value, setValue] = useState('Editable value'); | ||
| return ( | ||
| <div className="w-56"> | ||
| <LockableValueField | ||
| fieldType="string" | ||
| value={value} | ||
| onValueChange={setValue} | ||
| locked={false} | ||
| showFieldActions={false} | ||
| /> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔵 Needs a closer look
Six finalized findings remain, including four moderate issues and two nits.
Review details
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
apps/storybook/src/patterns/FieldTypes.stories.tsx:676
- The dagger is conveyed only by a non-focusable
<sup title=...>. Assistive technologies can expose the literal†without the title, leaving users who cannot see the legend unable to learn what the confidence marker means. Give the marker an accessible name, or associate it with the legend usingaria-describedby.
apps/storybook/src/patterns/FieldTypes.stories.tsx:587
ModeExampleis used for the two Binding state examples, but it never suppliesonModeChange.LockableValueFielddisables itsChoose value typecontrol when that callback is absent, so these live table examples cannot switch between Fixed and Expression even though the page promises interactive mode toggles. Keep a local mode state and pass it throughonModeChange.
mode={mode}
apps/storybook/src/patterns/FieldTypes.stories.tsx:119
- This support text is inaccurate:
LockableValueFieldrendersintegeras a plain<input type="number">withoutstep={1}or integer-specific validation, so decimal text can still be entered. Reword the row to describe the actual generic number input, or add and document real integer validation in the component.
support: 'Renders via integer today, but the control does not allow decimals.',
apps/storybook/src/patterns/FieldTypes.stories.tsx:691
- This renders an em dash in user-facing Storybook copy. The repository's Storybook documentation convention prohibits em dashes; use a text placeholder instead.
{row.action ?? '—'}
apps/storybook/src/patterns/FieldTypes.stories.tsx:996
- The examples rendered through this table include the
InputandInputGroupInputdemos, but none of those controls has an associated<Label>or anaria-label(the row'sStatetext is not a form label). The read-only and disabled examples therefore have no accessible name, so a screen-reader user cannot tell what each live control demonstrates. Pass an accessible name throughStateRow/the example components or add visually-hidden labels to these demos.
<Example />
apps/storybook/src/patterns/FieldTypes.stories.tsx:566
- This comment names
aria-label="File upload area", but theFieldTypeExamplehas no label andLockableValueFieldderives the file upload label from its field label, so this demo is actually namedFile value. Keep the comment accurate (or passfileUploadAriaLabelexplicitly); otherwise the accessibility behavior is being documented against the wrong name.
// the accessible name (aria-label="File upload area") is unaffected, it does
// not depend on this visible text.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
StatesTable now puts each state across as a column (label as the header, live example and notes stacked underneath) instead of down as a row. All three tables that use it (Input, Input Group, LockableValueField states) happen to have exactly 4 states, so this cuts each from 4 stacked rows to 2, considerably shorter vertically with the same content. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved accessibility, interaction, and documentation accuracy issues remain.
Review details
Suppressed comments (9)
Previously missed (1) — in code that hasn't changed since the last review.
apps/storybook/src/patterns/FieldTypes.stories.tsx:859
- The live
Inputexamples in this section have no accessible name: a placeholder or current value is not a label. Screen-reader users therefore encounter several indistinguishable controls on this documentation page, and an accessibility audit will flag them. Add anaria-labelor a visible, associatedLabelto each primitive example (including the otherInputstates below).
This issue also appears on line 914 of the same file.
apps/storybook/src/patterns/FieldTypes.stories.tsx:119
LockableValueFieldrendersintegeras<input type="number">withoutstep, validation, or a filtering handler, so a user can type1.5andonValueChangereceives it. This copy makes the audit inaccurate and can lead readers to infer a decimal gap that the current control does not actually prevent; either describe the lack of integer enforcement here or enforce it in the component before keeping this claim.
support: 'Renders via integer today, but the control does not allow decimals.',
apps/storybook/src/patterns/FieldTypes.stories.tsx:918
- The
InputGroupInputdemos below are also unlabeled; the leading search/lock icon and placeholder do not provide an accessible name for the input. Add anaria-labelor associated visibleLabelto eachInputGroupInputexample so this reference page remains usable with assistive technology.
<InputGroupInput
placeholder="Search..."
value={value}
onChange={(e) => setValue(e.target.value)}
/>
apps/storybook/src/patterns/FieldTypes.stories.tsx:588
LockableValueFielddisables its mode menu whenonModeChangeis absent, so both Binding state examples rendered byModeExampleare static and cannot exercise the Fixed/Expression toggle. Keep the mode in local state and passonModeChange(asFieldTypeExampledoes) so these examples fulfill the page's interactive reference/test-plan promise.
mode={mode}
showFieldActions={false}
apps/storybook/src/patterns/FieldTypes.stories.tsx:783
- Although this example is labeled "Unlocked, lock shown", it does not pass
onLockedChange;LockToggleButtontherefore deliberately renders the lock button disabled. The demo cannot demonstrate the lock affordance it documents. Add local locked state andonLockedChangeso the example can toggle between the two states.
<LockableValueField
fieldType="string"
value={value}
onValueChange={setValue}
locked={false}
showFieldActions={false}
apps/storybook/src/patterns/FieldTypes.stories.tsx:1178
- The dagger legend does not match the rows it marks. Several † rows (
collection,stringArray...,checkboxGroup, andradioGroup) come fromintegration-service WidgetType, not only a schema declaration; the intended distinction is that flow-workbench has not been confirmed to render them separately. Describing all of them as "only a schema-level type name" misclassifies the source and conflicts with the page's stated confidence-flag semantics. Reword this as an unconfirmed distinct control.
<span className="font-mono text-foreground">†</span> is different: it is only a
schema-level type name, and it is not confirmed that flow-workbench itself treats it as
a separately-rendered control. For those rows the gap may not be “Apollo Wind is
missing this,” it may be “no one has decided this is a real distinction yet,
in either place.” See that row’s recommended action before treating it as a
apps/storybook/src/patterns/FieldTypes.stories.tsx:1220
- When
onlySupportedis enabled,visibleSectionscontains only supported rows, sogapCount(section.rows)is always zero for every remaining category and the header incorrectly changes toall supported(for example, Temporal still has three omitted gaps). Compute this count from the unfilteredALL_SECTIONSrow list while filtering only the rendered rows.
{gapCount(section.rows) === 0
? 'all supported'
: `${gapCount(section.rows)} gap${gapCount(section.rows) === 1 ? '' : 's'}`}
apps/storybook/src/patterns/FieldTypes.stories.tsx:675
- The marker's meaning is exposed only through
titleon a non-focusable<sup>. That hover-only mechanism is not reliably announced to screen readers or available to keyboard/touch users, so the eight-row confidence flag is inaccessible. Add visually hidden explanatory text while keeping the visible dagger.
<sup
className="ml-0.5 cursor-help text-muted-foreground"
title="Not confirmed as a distinct, separately-rendered type in flow-workbench either, only a schema-level type name. See Recommended action."
apps/storybook/src/patterns/FieldTypes.stories.tsx:949
- The locked example renders a focusable
InputGroupButtonwith noonClickor disabled state. It announces an action named "Locked", but activation has no effect, which is misleading and leaves a dead control in the reference page. Use a noninteractive lock icon for this read-only state or wire it to the binding behavior used by the referenced recipe.
<InputGroupButton icon size="3xs" aria-label="Locked">
<Lock size={12} />
</InputGroupButton>
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Every table on this page wrapped its Table in a div with overflow-hidden, meant to clip the table's square corners to match the rounded-lg border. But the Visual example / Example columns embed w-56 live components, and 4 columns of those (or 2 stacked Fixed/Expression ones) routinely make the table wider than its ~960px container. overflow-hidden clipped that overflow instead of letting it scroll, silently hiding whatever landed in the last column (confirmed: Input's Invalid and Input Group's Locked (read-only) states were being cut off, not just those two, 7 tables total on this page were overflowing their container). Removed overflow-hidden from both CategoryTable's and StatesTable's wrapper divs. Table already provides its own scrollable container (overflow-auto), so content that doesn't fit is now reachable by scrolling instead of invisibly cut. Traded off perfectly square corners peeking past the rounded border in the rare unscrolled case for never hiding content, worth it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Address the unresolved accessibility, interaction, validation, and responsive-layout issues.
Review details
Suppressed comments (7)
Previously missed (4) — in code that hasn't changed since the last review.
apps/storybook/src/patterns/FieldTypes.stories.tsx:861
- This bare
Inputhas no programmatic label;placeholderis not a substitute for an accessible name, so this reference example will fail form accessibility checks. Add anaria-labelor aLabel/htmlForpair, as the other field guidance examples do.
apps/storybook/src/patterns/FieldTypes.stories.tsx:920 - The leading search icon is decorative and the
InputGroupInputhas only a placeholder, so this live search example has no programmatic label. Add anaria-labelor a visible label for the grouped input.
apps/storybook/src/patterns/FieldTypes.stories.tsx:952 - Unlike
LockToggleButton, which disables itself when noonLockedChangeis supplied, this example renders an enabled button with no click handler. It presents a lock affordance that users can activate but that never changes state; make it disabled or wire it to a stateful toggle.
apps/storybook/src/patterns/FieldTypes.stories.tsx:696 - This is user-facing Storybook copy, so the em-dash fallback violates the repository's Storybook documentation convention. Use a plain-text placeholder such as
No action neededfor supported rows instead.
apps/storybook/src/patterns/FieldTypes.stories.tsx:883
- The example displays a non-empty value but reports
This field is required., so it contradicts the state it is documenting and implies an incorrect validation rule. Use an empty value for a required-field example or change the message to match the shown invalid value.
const [value, setValue] = useState('Invalid value');
return (
<Input
className="w-56"
value={value}
onChange={(e) => setValue(e.target.value)}
error="This field is required."
apps/storybook/src/patterns/FieldTypes.stories.tsx:942
- The example displays a non-empty value but reports
This field is required., so it contradicts the state it is documenting and implies an incorrect validation rule. Use an empty value for a required-field example or change the message to match the shown invalid value.
const [value, setValue] = useState('Invalid value');
return (
<InputGroup className="w-56" error="This field is required.">
apps/storybook/src/patterns/FieldTypes.stories.tsx:735
- At widths below
xl, this renders four columns (fromsm) while eachFieldTypeExamplehas a fixedw-56(224px). For example, a 704px content area atsmgives each track only about 164px, so the demos overflow into adjacent cells rather than forming a usable grid. Use breakpoints that keep one/two columns until four demos fit, or make the demo width fluid.
<div className="grid grid-cols-2 gap-4 sm:grid-cols-4">
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
…on and binding tables
Structural reorganization plus new content, based on review feedback:
- Fixed the actual clipping bug: states tables now use table-fixed with
w-full examples instead of fixed w-56 pixel widths, so all 4 columns
show evenly with nothing pushed past the edge (previously
overflow-hidden was silently cutting off the last column, e.g.
Input's Invalid and Input Group's Locked states).
- Split the old single "LockableValueField states" table into three:
no left icon (showLock={false}), with left icon (showLock={true}),
and assignment & binding (mirrors the real "Assignment & Binding"
reference example in lockable-value-field.stories.tsx: leadingAddon
"=" instead of the lock icon, mode always expression). Dropped
"What each type looks like" now that these cover the same ground
with clearer, comparable state tables.
- Reorganized the whole page into three top-level parts for clarity:
Overview (why this exists), Types (Input, Input Group, the three
LockableValueField tables, Supported today), and LockableValueField
status (the status legend, the only-supported filter, and the
gap-tracking category accordion). Supported today moved into the
status part, it's a status statement, not a component-anatomy demo.
- Added the same Expand all / Collapse all + independently-collapsible
section behavior to the Types part that the status part already
had, with its own toggle state so the two accordions don't interfere.
- Consistent 40px spacing between grouped sub-sections (no divider),
a full Divider only between the three main parts.
- Expand all / Collapse all now sits on the same row as the "Types"
heading, right-aligned, via a new optional actions slot on PartTitle.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Several examples have nonfunctional controls, and two documentation descriptions need correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (8)
apps/storybook/src/patterns/FieldTypes.stories.tsx:898
- This control is exposed as a button with an accessible “Open expression editor” label, but it has no
onClickand no editor/popover is rendered. Clicking or keyboard-activating the documented custom-expression-editor example therefore does nothing, contrary to the page's interactive-addon behavior. Wire it to a real editor/stateful demo, or render the glyph as non-interactive content instead of a button.
<button
type="button"
aria-label="Open expression editor"
className="grid size-7 place-items-center border-l border-border text-foreground-subtle transition hover:bg-surface-overlay hover:text-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2"
apps/storybook/src/patterns/FieldTypes.stories.tsx:1100
- This is a focusable button with an accessible name but no click handler and no disabled state, so the read-only recipe gives keyboard and pointer users an interactive control that silently does nothing. Since this row is documenting a static locked state, use a decorative lock icon, or wire the button to a real lock/binding interaction.
<InputGroupButton icon size="3xs" aria-label="Locked">
<Lock size={12} />
</InputGroupButton>
apps/storybook/src/patterns/FieldTypes.stories.tsx:755
- The shared examples used by the “with left icon” states pass
showLock={true}but omitonLockedChange;LockToggleButtonconsequently disables the lock control when the callback is absent. They also omitonModeChange, so the value-mode menu is disabled. The page describes these as built-in affordances and the PR test plan calls the live examples interactive, so add local lock/mode state handlers or explicitly present these as non-interactive snapshots.
<LockableValueField
fieldType="string"
value={value}
onValueChange={setValue}
locked={false}
showLock={showLock}
showFieldActions={false}
apps/storybook/src/patterns/FieldTypes.stories.tsx:594
ModeExamplepasses a fixedmodebut noonModeChange;LockableValueFieldconsequently disables its Fixed/Expression menu. The two mode examples are therefore not toggleable even though this page describes the examples as interactive and the test plan calls out those toggles. Keep the initial mode in local state and pass its setter asonModeChange.
mode={mode}
apps/storybook/src/patterns/FieldTypes.stories.tsx:1188
- At narrow viewports this combination shrinks each column to roughly a quarter of the page while
HEADER_CELL_CLASSforceswhitespace-nowrap; the sharedTableHeadalso appliesoverflow-hidden. Labels such as “Custom expression editor” are therefore clipped instead of wrapping, despite the comment and test plan promising no clipping. Allow header wrapping in this table (or give the table a responsive minimum width and deliberate scroll behavior).
<Table className="table-fixed">
<TableHeader>
<TableRow className="hover:bg-transparent">
{rows.map(({ label }) => (
<TableHead key={label} className={cn(HEADER_CELL_CLASS, columnClass)}>
apps/storybook/src/patterns/FieldTypes.stories.tsx:991
Inputdoes not define abelowValueprop; that prop belongs toLockableValueField. This description tells consumers that the plain Input exposes the same API, which is incorrect and can lead them to look for a nonexistent prop. Please describebelowValueas a LockableValueField feature instead.
description: 'belowValue adds context below the field, same prop the plain Input uses.',
apps/storybook/src/patterns/FieldTypes.stories.tsx:119
- The
integerimplementation uses an<input type="number">without astep, parsing, or validation, so decimal text can still be entered and propagated throughonValueChange(a default step only affects form validity). Saying the control “does not allow decimals” is inaccurate; describe the current limitation as lacking decimal-specific validation/formatting instead.
support: 'Renders via integer today, but the control does not allow decimals.',
apps/storybook/src/patterns/FieldTypes.stories.tsx:1364
- The legend says every
†row is “only a schema-level type name”, but several flagged rows (collection,stringArray,checkboxGroup, andradioGroup) are sourced from the integration-serviceWidgetTypecatalog. The flag actually means renderer distinction is unconfirmed, so rewrite this sentence to match theflowWorkbenchConfirmedsemantics.
<span className="font-mono text-foreground">†</span> is different: it is only a
schema-level type name, and it is not confirmed that flow-workbench itself treats it as
a separately-rendered control. For those rows the gap may not be “Apollo Wind is
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
| locked={false} | ||
| mode={mode} | ||
| onModeChange={setMode} | ||
| showFieldActions={false} |
…sion-editor icon - New "LockableValueField, insert variable" table in Types: Default, Locked, No variables, Insert variable only. Confirmed the disabled logic is real, not just visual: locked disables the button because onValueChange becomes undefined, empty variables disables it for a separate reason, both read the same to a user but are distinct behaviors worth documenting side by side. - The "Assignment & Binding" table's 4th column (custom expression editor) copied the real reference story's FunctionAddon verbatim: a bordered box around a literal "ƒ" glyph with a left-border divider. That's the only trailing icon on the whole page styled that way, every other one (mode-switch, Insert variable, lock toggle) is a plain ghost icon with no divider. Restyled it to match those, using the same Code2 icon the real mode-switch uses for expression mode, so it stops reading as a rendering bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved issues affect documented interactivity, the expression-editor action, responsive headers, and integer-input accuracy.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (7)
Previously missed (1) — in code that hasn't changed since the last review.
apps/storybook/src/patterns/FieldTypes.stories.tsx:755
- The lockable state helpers do not provide
onLockedChange.LockableValueFieldintentionally disables its lock button when that callback is absent, so the "with left icon" table shows a disabled affordance rather than the documented lock toggle (and the locked state has the same problem). Add controlled locked state/callbacks or label these as static snapshots.
This issue also appears on line 1188 of the same file.
apps/storybook/src/patterns/FieldTypes.stories.tsx:595
- These examples pass
modeas a controlled prop but omitonModeChange.LockableValueFieldtherefore renders the Fixed/Expression button disabled, so the binding-state examples are not toggleable despite the page's interactive-example/test-plan claim. Add local mode state and pass it toonModeChange(and apply the same pattern to the other state helpers).
mode={mode}
showFieldActions={false}
apps/storybook/src/patterns/FieldTypes.stories.tsx:1190
- This
InputGroupButtonhas no handler and is not disabled, so the locked recipe exposes a focusable button that announces "Locked" but does nothing. Render the lock as a decorative icon for this static read-only recipe, or wire it to the intended lock/binding behavior.
<InputGroupButton icon size="3xs" aria-label="Locked">
<Lock size={12} />
</InputGroupButton>
apps/storybook/src/patterns/FieldTypes.stories.tsx:991
belowValueis aLockableValueFieldprop; the plainInputcomponent does not accept it. Describing this as the same prop used byInputmakes this reference point consumers to an API that does not exist. Remove that comparison or describe the content as specific toLockableValueField.
label: 'With helper text',
description: 'belowValue adds context below the field, same prop the plain Input uses.',
Example: BindingWithHelperTextExample,
apps/storybook/src/patterns/FieldTypes.stories.tsx:1350
- The PR description promises that all three top-level parts are independently collapsible with Expand all/Collapse all, but Overview is a plain section here; only Types and LockableValueField status have accordions and bulk controls. Either make Overview collapsible or update the description/test plan to match the two-accordion implementation.
<PartTitle>Overview</PartTitle>
apps/storybook/src/patterns/FieldTypes.stories.tsx:503
- This shared header class forces
h-9andwhitespace-nowrap, while theTableHeadprimitive also appliesoverflow-hidden. In the four-columnStatesTable, the columns are fixed to 25%, so at narrow Storybook widths headings such as "Insert variable only" are clipped instead of wrapping; use a responsive/wrapping header (for exampleh-auto whitespace-normal) or a deliberate min-width/scroll behavior.
'h-9 whitespace-nowrap bg-muted/40 text-[11px] font-semibold uppercase tracking-wide';
apps/storybook/src/patterns/FieldTypes.stories.tsx:121
- This description is inaccurate for the current implementation:
LockableValueFieldrenders integer fixed mode as an<input type="number">withoutstep=1or integer validation, so decimal values can still be entered. Either enforce integer-only input in the component or describe the current behavior accurately here.
type: 'number / float / double',
source: 'hitl-schema-types (float, double), JSON Schema number',
support: 'Renders via integer today, but the control does not allow decimals.',
status: 'needs-type',
action: 'Add a number fieldType for decimal values, separate from integer.',
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
| function FunctionAddon() { | ||
| return ( | ||
| <InputGroupButton icon size="3xs" aria-label="Open expression editor"> | ||
| <Code2 /> | ||
| </InputGroupButton> |
Summary
New Storybook page at
Apollo Wind/Forms/Field Types, sitting next toField guidance. Documents every field-level building block used on a form and cross-references every value type flow-workbench needs against what LockableValueField supports today, so engineers have one place to check before adding a new type case instead of improvising.Page structure, three top-level parts, each independently collapsible with Expand all / Collapse all:
lockable-value-field.stories.tsx, leadingAddon=instead of the lock icon, mode always expression, including the custom-expression-editor trailing-addon variant.variablesprop and its Insert-variable popover (normally suppressed everywhere else on the page viashowFieldActions={false}), across Default / Locked / No variables / Insert-variable-only, confirming locked and empty-variables disable the button for two different real reasons.FIELD_TYPE_META), the status legend (Supported/Needs fieldType/Needs component, plus a†flag for rows where it's not confirmed flow-workbench itself treats the type as distinct), an "Only show supported" filter, and the gap-tracking category accordion (Temporal, Numeric, Boolean, Text/string formats, Security, Object/collection, File/media, Choice controls, Resource reference, Rich/composite, Loose/fallback, Binding state — now including Insert-variable's supported status and a companion gap row for inline reference autocomplete-while-typing, which is not modeled).All live examples are real, interactive
LockableValueField/Input/Input Groupinstances, never fabricated mockups; gap rows show a "No control yet" placeholder instead.Posting as a draft to get design/eng feedback on the three-part structure, the icon/binding/insert-variable split, the
†confidence flag, and whether the recommended actions per gap type are right before this becomes the thing people are pointed to.Test plan
Apollo Wind/Forms/Field Types→Documentation)=addon, and Insert-variable's popover†read correctly against the legend calloutpnpm biome checkpasses on the new file🤖 Generated with Claude Code