UX-1338 — Redpanda Connect Pipeline (RPCN) Visual View & Editor#2553
UX-1338 — Redpanda Connect Pipeline (RPCN) Visual View & Editor#2553SpicyPete wants to merge 22 commits into
Conversation
SpicyPete
requested review from
a team,
c-julin,
eblairmckee and
malinskibeniamin
✅ Clean — no registry drift, off-token colours, or ad-hoc classesApp:
Generated by lookout audit-changes. |
SpicyPete
changed the title
|
@claude review |
|
Claude finished @SpicyPete's task in 1m 24s —— View job Code Review — UX-1338 RPCN Visual EditorReviewed across 5 parallel passes (parser/layout, canvas, inspector/store, yaml/utils, panels/shared-UI). This is a large but carefully written PR — effect cleanup, undo/redo, the save-flush path, and the shared-UI signature changes all check out. Below are the findings worth acting on, most impactful first. 🟠 Correctness1. const inputType = Object.keys(inputObj)[0]; // ← should be firstKey(inputObj)Everywhere else in this file uses 2. Switch-case condition draft leaks across sibling cases → silent misrouting — const initial = typeof caseObject.check === 'string' ? caseObject.check : '';
const [check, setCheck] = useState(initial);
useEffect(() => setCheck(initial), [initial]); // only resyncs when the primitive value changes
3. 🟡 Performance (large graphs)4. Scope-region work re-runs on pan/zoom and hover —
Fix direction: derive 🟢 Minor / nits
Also worth noting the PR's own stated deferral: the delete-guard for nodes with dependents is still a TODO, and Nothing here is a merge-blocker for a feature-flagged surface, but I'd fix #1 and #2 before wider rollout — both silently corrupt/misroute saved config. Nice work on the parser test coverage and the paint-cap tiling. |
|
#2554 |
| <ConnectTiles | ||
| className="px-0 py-0" | ||
| components={components} | ||
| componentTypeFilter={typeFilter} | ||
| gridCols={3} | ||
| hideHeader | ||
| onChange={onAddConnector} | ||
| searchPlaceholder={searchPlaceholder} | ||
| variant="ghost" | ||
| /> |
There was a problem hiding this comment.
we should make an action item that once this is GA and well tested we remove the old RPCN UXs. potentially Q3 or Q4
| }) => { | ||
| const { labels, onCreateResource } = useContext(ResourceFieldContext); | ||
| const options = labels[kind]; | ||
| const current = value ?? ''; |
There was a problem hiding this comment.
nit: could just set the initial value on line 170 value = ''
| <SelectValue placeholder="Select a resource…" /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {isMissing ? <SelectItem value={current}>{current} (missing)</SelectItem> : null} |
There was a problem hiding this comment.
what's the edge case for a missing item? not clear to me
| <SelectItem value={CREATE_RESOURCE_VALUE}> | ||
| <span className="flex items-center gap-1.5 text-primary"> | ||
| <Plus className="size-3.5" /> | ||
| Create new {kind === 'cache' ? 'cache' : 'rate limit'}… | ||
| </span> | ||
| </SelectItem> |
There was a problem hiding this comment.
99% sure we have a component for this.. I would look at what ui registry combobox does... it may or may not be exported as a subcomponent
| ); | ||
| }; | ||
|
|
||
| const SCALAR_TYPES = new Set(['string', 'int', 'float', 'bool']); |
There was a problem hiding this comment.
i feel like a constant exists for this in schema.ts (one of them...)
| // A single editable value: a primitive (string/int/float/bool), an enum select, or a | ||
| // resource reference (stored as a string). | ||
| function isScalarField(spec: RawFieldSpec): boolean { | ||
| return ( | ||
| Boolean(spec.name) && | ||
| spec.kind === 'scalar' && | ||
| (SCALAR_TYPES.has(spec.type) || hasOptions(spec) || isResourceRefField(spec)) | ||
| ); | ||
| } |
There was a problem hiding this comment.
same here. when we parse the rpcn schema we have a few utils and consts for determining field types. would love to see those reused, or move these utils to schema.ts
There was a problem hiding this comment.
that said, since I'd love to see these utils generalized in schema.ts we should remove these unnecessary comments. when in doubt, give a util a better name, don't vomit comments unnecessarily (I am look at your, claude)
| const COMPONENT_FIELD_TYPES = new Set([ | ||
| 'input', | ||
| 'output', | ||
| 'processor', | ||
| 'cache', | ||
| 'rate_limit', | ||
| 'buffer', | ||
| 'metrics', | ||
| 'tracer', | ||
| 'scanner', | ||
| ]); |
There was a problem hiding this comment.
also exists in schema.ts
| function setInObj(obj: Record<string, unknown>, path: string[], value: unknown): void { | ||
| let cur = obj; | ||
| for (const key of path.slice(0, -1)) { | ||
| if (!cur[key] || typeof cur[key] !== 'object' || Array.isArray(cur[key])) { | ||
| cur[key] = {}; | ||
| } | ||
| cur = cur[key] as Record<string, unknown>; | ||
| } | ||
| cur[path.at(-1) as string] = value; | ||
| } | ||
|
|
||
| function deleteInObj(obj: Record<string, unknown>, path: string[]): void { | ||
| const parent = path.slice(0, -1).reduce<Record<string, unknown> | undefined>((cur, key) => { | ||
| const next = cur?.[key]; | ||
| return next && typeof next === 'object' ? (next as Record<string, unknown>) : undefined; | ||
| }, obj); | ||
| if (parent) { | ||
| delete parent[path.at(-1) as string]; | ||
| } | ||
| } | ||
|
|
||
| // Drop objects that became empty after clearing their fields, so the YAML stays tidy. | ||
| function pruneEmptyObjects(obj: Record<string, unknown>): void { | ||
| for (const [key, val] of Object.entries(obj)) { | ||
| if (val && typeof val === 'object' && !Array.isArray(val)) { | ||
| pruneEmptyObjects(val as Record<string, unknown>); | ||
| if (Object.keys(val).length === 0) { | ||
| delete obj[key]; | ||
| } |
There was a problem hiding this comment.
these seem like lodash util rewrites... I would be shocked if we didn't already have an utils/object file with helpers for this. if not... let's add one
| function coerceScalar(spec: RawFieldSpec, raw: string | boolean): string | number | boolean { | ||
| if (spec.type === 'bool') { | ||
| return Boolean(raw); | ||
| } | ||
| const text = String(raw); | ||
| // Interpolations (`${ENV}`, secrets, Bloblang) are valid even in numeric fields — keep verbatim | ||
| // rather than coercing to NaN (→ '' → dropped on commit). Matches numericHint. | ||
| if ((spec.type === 'int' || spec.type === 'float') && text.includes('${')) { | ||
| return text; | ||
| } | ||
| if (spec.type === 'int') { | ||
| const n = Number.parseInt(text, 10); | ||
| return Number.isNaN(n) ? '' : n; | ||
| } | ||
| if (spec.type === 'float') { | ||
| const n = Number(text); | ||
| return text === '' || Number.isNaN(n) ? '' : n; | ||
| } | ||
| return text; | ||
| } | ||
|
|
||
| function coerceArrayItems(spec: RawFieldSpec, text: string): unknown[] { | ||
| const lines = text | ||
| .split('\n') | ||
| .map((l) => l.trim()) | ||
| .filter((l) => l !== ''); | ||
| if (spec.type === 'int' || spec.type === 'float') { | ||
| return lines.map(Number).filter((n) => !Number.isNaN(n)); | ||
| } | ||
| return lines; | ||
| } |
There was a problem hiding this comment.
again.. more schema.ts rewrites
| const FieldLabel = ({ spec, htmlFor }: { spec: RawFieldSpec; htmlFor?: string }) => ( | ||
| <div className="flex items-center gap-2"> | ||
| <Label className="font-medium text-sm" htmlFor={htmlFor}> | ||
| {spec.name} | ||
| </Label> | ||
| {checkRequired(spec) ? ( | ||
| <span aria-hidden className="text-destructive text-xs" title="Required"> | ||
| * | ||
| </span> | ||
| ) : null} | ||
| {spec.type && spec.type !== 'string' ? <span className="text-muted-foreground text-xs">{spec.type}</span> : null} | ||
| {spec.defaultValue ? ( | ||
| <span className="text-muted-foreground text-xs"> | ||
| default: <span className="font-mono">{spec.defaultValue}</span> | ||
| </span> | ||
| ) : null} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
iirc we have a component for Required... but then again that may have been adp-ui...
regardless.. worth elevating this to a shared components directory rather than one-offing this. would be very useful in react-hook-form instances.
| const FieldDescription = ({ spec }: { spec: RawFieldSpec }) => | ||
| spec.description ? ( | ||
| <Text className="text-muted-foreground" variant="bodySmall"> | ||
| {spec.description} | ||
| </Text> | ||
| ) : null; |
There was a problem hiding this comment.
same comment as above.
There was a problem hiding this comment.
worth noting I like to have both field-description as well as field-helper-text (the latter being positioned below the field, mutually exclusive with field-error)
this is pseudo code for the sake of discussion. would be nice to elevate these kinds of things to be common components so future console UIs can reuse them. and eventually upstream to ui registry as typography utility classes 😏
| const SecretInput = (props: { id?: string; value: string; onChange: (value: unknown) => void; required?: boolean }) => { | ||
| const [show, setShow] = useState(false); | ||
| const isReference = props.value.includes('${'); | ||
| return ( | ||
| <div className="flex items-center gap-1.5"> | ||
| <Input | ||
| aria-required={props.required || undefined} | ||
| autoComplete="off" | ||
| id={props.id} | ||
| onChange={props.onChange} | ||
| type={show || isReference ? 'text' : 'password'} | ||
| value={props.value} | ||
| /> | ||
| {isReference ? null : ( | ||
| <Button | ||
| aria-label={show ? 'Hide value' : 'Show value'} | ||
| onClick={() => setShow((v) => !v)} | ||
| size="icon-sm" | ||
| type="button" | ||
| variant="ghost" | ||
| > | ||
| {show ? <EyeOff className="size-3.5" /> : <Eye className="size-3.5" />} | ||
| </Button> | ||
| )} | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
we have a Input type="password" in the registry already, prefer using that... or if we need something more custom this deserves to be its own component.
Actually... I think the registry also has a SecretSelector component, too, that's compatible with auto-form/react-hook-form... worth investigating.
| value: string | boolean; | ||
| onChange: (value: unknown) => void; |
There was a problem hiding this comment.
seems a little off.... should was have a Value: string | boolean and reuse for the onChange value type?
| function numericHint(spec: RawFieldSpec, value: string | boolean): string | null { | ||
| const text = String(value ?? '').trim(); | ||
| if (text === '' || typeof value === 'boolean' || text.includes('${')) { | ||
| return null; | ||
| } | ||
| if (spec.type === 'int' && !INT_VALUE_RE.test(text)) { | ||
| return "Not a valid integer — this change won't be saved until fixed."; | ||
| } | ||
| if (spec.type === 'float' && Number.isNaN(Number(text))) { | ||
| return "Not a valid number — this change won't be saved until fixed."; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
again with reusability. these could be lovely helpers for auto-form instances
| } | ||
|
|
||
| // biome-ignore lint/suspicious/noTemplateCurlyInString: a literal Connect secret reference, not a JS template | ||
| const SECRET_REF_EXAMPLE = '${secrets.MY_SECRET}'; |
There was a problem hiding this comment.
also pretty sure we have a function for this already
| <Collapsible className="rounded-md border border-border/60" defaultOpen={defaultOpen}> | ||
| <CollapsibleTrigger className="group flex w-full items-center justify-between px-3 py-2 text-left"> | ||
| <Text variant="bodyStrongMedium">{label}</Text> | ||
| <ChevronDown | ||
| className={cn('size-4 text-muted-foreground transition-transform group-data-[panel-open]:rotate-180')} | ||
| /> | ||
| </CollapsibleTrigger> | ||
| <CollapsibleContent className="flex flex-col gap-4 px-3 pt-1 pb-3">{children}</CollapsibleContent> |
There was a problem hiding this comment.
this is basically ui registry Accordion. worth looking at whether it's a viable substitute rather than yet another one-off
| const ordered = [ | ||
| ...formFields.filter((f) => checkRequired(f) && !f.advanced), | ||
| ...formFields.filter((f) => !(checkRequired(f) || f.advanced)), | ||
| ...formFields.filter((f) => f.advanced), | ||
| ]; |
There was a problem hiding this comment.
nit: a flapMap + predicate util would be lovely to use here.
| {isListValued && hasChildList ? ( | ||
| <ChildItemsList | ||
| items={childItems as InspectorChildItem[]} | ||
| label="Cases" | ||
| onSelect={onSelectChild as (item: InspectorChildItem) => void} | ||
| /> | ||
| ) : null} | ||
| {isListValued && !hasChildList ? ( | ||
| <div className="rounded-md border border-border/60 border-dashed px-3 py-2"> | ||
| <Text className="text-muted-foreground" variant="bodySmall"> | ||
| This component's items (cases / processors) are edited on the canvas — select one to edit it. | ||
| </Text> | ||
| </div> | ||
| ) : null} | ||
|
|
||
| {isListValued ? null : required.map((f) => <SchemaField control={control} key={f.name} path={[]} spec={f} />)} | ||
|
|
||
| {!isListValued && optional.length > 0 ? ( | ||
| <FieldGroup label="Optional"> | ||
| {optional.map((f) => ( | ||
| <SchemaField control={control} key={f.name} path={[]} spec={f} /> | ||
| ))} | ||
| </FieldGroup> | ||
| ) : null} | ||
|
|
||
| {!isListValued && advanced.length > 0 ? ( | ||
| <FieldGroup defaultOpen={false} label="Advanced"> | ||
| {advanced.map((f) => ( | ||
| <SchemaField control={control} key={f.name} path={[]} spec={f} /> | ||
| ))} | ||
| </FieldGroup> | ||
| ) : null} | ||
|
|
||
| {!isListValued && componentFields.length > 0 && hasChildList ? ( | ||
| <ChildItemsList | ||
| items={childItems as InspectorChildItem[]} | ||
| label="Steps" | ||
| onSelect={onSelectChild as (item: InspectorChildItem) => void} | ||
| /> | ||
| ) : null} | ||
| {!isListValued && componentFields.length > 0 && !hasChildList ? ( | ||
| <div className="rounded-md border border-border/60 border-dashed px-3 py-2"> | ||
| <Text className="text-muted-foreground" variant="bodySmall"> | ||
| {componentFields.map((f) => f.name).join(', ')}{' '} | ||
| {componentFields.length === 1 ? 'is a nested component' : 'are nested components'} — select{' '} | ||
| {componentFields.length === 1 ? 'it' : 'them'} on the canvas to edit. | ||
| </Text> | ||
| </div> | ||
| ) : null} | ||
|
|
||
| {!isListValued && showRaw ? ( | ||
| <FieldGroup defaultOpen={false} label="Other settings (YAML)"> | ||
| <Controller | ||
| control={control} | ||
| name="raw" | ||
| render={({ field }) => { | ||
| const invalid = field.value.trim() !== '' && parseRawSection(true, field.value) === null; | ||
| return ( | ||
| <div className="flex flex-col gap-1.5"> | ||
| <div className="h-[200px] overflow-hidden rounded-md border border-border"> | ||
| <YamlEditor | ||
| onChange={(v) => field.onChange(v || '')} | ||
| options={{ minimap: { enabled: false } }} | ||
| transparentBackground | ||
| value={field.value} | ||
| /> | ||
| </div> | ||
| {invalid ? ( | ||
| <Text className="text-destructive" variant="bodySmall"> | ||
| Invalid YAML — these settings won't be saved until fixed. | ||
| </Text> | ||
| ) : null} | ||
| </div> | ||
| ); | ||
| }} |
There was a problem hiding this comment.
this would be a lot more readable if we hoisted this to a constant and returned most -> least specific conditionals.
| const invalid = field.value.trim() !== '' && parseRawSection(true, field.value) === null; | ||
| return ( | ||
| <div className="flex flex-col gap-1.5"> | ||
| <div className="h-[200px] overflow-hidden rounded-md border border-border"> |
There was a problem hiding this comment.
why overflow-hidden? seems like a broken UI for longer (over 200px high) yaml configs
There was a problem hiding this comment.
This is for the extra yaml field within the side-panel, so it needs some height set and overflow-hidden, I will increase the height so more can fit in, but the extra yaml field for this is likely rarely used
| /> | ||
| ) : null} | ||
| {!isListValued && componentFields.length > 0 && !hasChildList ? ( | ||
| <div className="rounded-md border border-border/60 border-dashed px-3 py-2"> |
There was a problem hiding this comment.
I'd be curious to see a screenshot of this.... dashed borders are typically used for placeholder/empty/you-could-add-something-optional form sections
| {danglingRef && !readOnly ? ( | ||
| <DanglingRefBanner onCreate={handleCreateMissingResource} refLabel={danglingRef.ref} /> | ||
| ) : null} | ||
| {(() => { |
There was a problem hiding this comment.
minor: can we lift this to a constant... I lean against inline function calls just to render more jsx... screams that it should be its own subcomponent.. or const within component body
| const conditionSection = | ||
| caseTarget && caseObject ? ( | ||
| // Key by the case's path so selecting a sibling remounts + resets the draft (see above). | ||
| <CaseConditionSection | ||
| caseObject={caseObject} | ||
| error={conditionError} | ||
| key={JSON.stringify(editTargetPath(caseTarget))} | ||
| onConfigChange={readOnly ? undefined : reportConditionDraft} | ||
| readOnly={readOnly} | ||
| /> | ||
| ) : null; | ||
| if (readOnly) { | ||
| return ( | ||
| <> | ||
| {conditionSection} | ||
| <ReadOnlyComponent component={component} /> | ||
| </> | ||
| ); | ||
| } | ||
| if (useForm && spec) { |
There was a problem hiding this comment.
my earlier comment about early returns for most -> least specific conditions... this is a great example of that. let's hoist this to outside the render
| const InspectorLintErrors = ({ hints }: { hints: LintHint[] }) => ( | ||
| <div className="shrink-0 border-destructive/30 border-b bg-destructive/5 px-4 py-3"> | ||
| <div className="mb-1 flex items-center gap-1.5 text-destructive"> | ||
| <AlertCircle className="size-4 shrink-0" /> | ||
| <Text as="span" className="text-destructive" variant="bodyStrongMedium"> | ||
| {hints.length === 1 ? '1 problem' : `${hints.length} problems`} | ||
| </Text> | ||
| </div> | ||
| <ul className="flex flex-col gap-1"> | ||
| {hints.map((hint, i) => ( | ||
| <li className="text-destructive text-xs" key={`${hint.line}-${hint.column}-${i}`}> | ||
| {hint.hint} | ||
| {hint.line > 0 ? <span className="text-destructive/70"> (line {hint.line})</span> : null} | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
looks like it could be a registry Alert
There was a problem hiding this comment.
Maybe, but in this instance then we'll have more boxes-in-boxes, so a custom full-width alert area makes sense to me
|
|
||
| // Write a component-config draft at `target`, cascading a resource-label rename to every component | ||
| // that references it (so the link is never silently broken). | ||
| function applyComponentDraft( |
There was a problem hiding this comment.
I am dying to know how claude decides whether a function should be a helper outside of the component body... vs a callback... this screams "should've been a callback"
There was a problem hiding this comment.
This is a pure function which doesn't rely on anything in the component, so keeping it out should be okay
2 participants
Summary
Adds a full visual editor for Redpanda Connect (RPCN) pipelines: an interactive, node-based canvas that renders a pipeline's YAML as a directed graph and lets you inspect and edit nodes without hand-writing YAML. It sits alongside the existing YAML editor as a new "Visual" lane.
The editor is feature-flagged and currently scoped to the embedded (Cloud) context — it activates only when
enableRpcnVisualEditorandenablePipelineDiagramsare on and the page is embedded. When enabled, editing opens on the Visual lane by default; otherwise the experience is unchanged (YAML lane).What's included
Canvas & layout
pipeline-flow-canvas.tsx,pipeline-flow-canvas-nodes.tsx) — renders the pipeline as a left-to-right DAG using React Flow + Dagre (@dagrejs/dagre, new dependency): input → processors → output, with control-flow constructs (switch,branch,try/catch,broker,fallback) drawn as split → case-lanes → merge, plus conditional/error/reference edge styling.pipeline-flow-parser.ts,pipeline-flow-meta.ts) — parses pipeline YAML into a flow tree and computes the layout with source-aligned ranks. Replaces the old sidebar diagram (pipeline-flow-diagram.tsxremoved).Editing
node-inspector.tsx,node-config-form.tsx) — side panel to view/edit a selected node's config via Registry Field forms (react-hook-form + Zod), drill into control-flow children (switch cases / steps), and edit a node's raw YAML in place.use-pipeline-editor-store.ts) — Zustand store with undo/redo; pending visual edits are flushed to YAML at save time.pipeline-canvas-command-palette.tsx,onboarding/connect-command-palette.tsx,component-aliases.ts) — quick actions (view in YAML, undo/redo, jump to node) and a connector/processor inserter with curated defaults, alias-aware search (e.g. "queue" →kafka_franz), and de-emphasis of deprecated/experimental components.Feedback surfaces
pipeline-structure-tree.tsx) — sidebar outline in document order with collapsible groups, roving-tabindex keyboard navigation, and click-to-reveal.pipeline-problems-panel.tsx,pipeline-lint.ts) — floating chip listing lint problems (mapped from backendLintHints by YAML line range, merged with save-error hints) and detected secret references; click a problem to jump to its node.pipeline-unsaved-panel.tsx,pipeline-diff.ts) — floating chip listing nodes whose config differs from the last save (signature-based diff); click to jump.editor-tips-bar.tsx,template-cta.tsx) — platform-aware keyboard-shortcut hints and a "start from a template" entry point for empty pipelines.Testing
Unit + integration tests accompany the new modules — flow parser/layout, lint mapping, dirty-diff, canvas render (incl. tall-region paint-cap), inspector, config form, structure tree, problems/unsaved panels, and command palettes.
type:check,lint, and the test suite are green.Known limitations / deferrals
How to test
Enable
enableRpcnVisualEditor+enablePipelineDiagrams, open a pipeline in the embedded editor, and confirm the Visual lane loads. Verify node selection/edit, undo/redo, save round-trips to YAML, and the problems/unsaved chips.Screenshots
Sidelane / YAML enhancements
Visual view