UX-1338 — Redpanda Connect Pipeline (RPCN) Visual View & Editor#2553
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 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
|
@claude review |
|
Claude finished @SpicyPete's task in 10m 32s —— View job Code Review — UX-1338 RPCN Visual EditorReviewed the current state of the branch (5 focused passes: parser/layout, canvas, inspector/store, yaml/utils, panels + a store/diff/lint deep-dive). All four correctness/perf findings from the earlier review are resolved, and the delete-guard TODO is now implemented. Below are the issues I found on this pass, most impactful first — each verified against callers and test fixtures. ✅ Previously-flagged issues — all fixed
🟠 Correctness1. const scannerName = Object.keys(newConfigObject)[0]; // 'csv'
const scannerConfig = newConfigObject[scannerName]; // the INNER fields
doc.setIn(['input', inputType, 'scanner'], scannerConfig); // ← drops the `csv:` key
doc.setIn(['input', inputType, 'scanner'], newConfigObject);Repro: input 2. An output 🟡 Minor / low-impact3. Help-comments target processor/resource index 4. Scanner help-comments never applied — 5. 6. The store/undo-redo, save-flush ordering, dirty-diff signatures, and lint line-range mapping all check out — nicely defensive. Nothing here blocks a feature-flagged surface, but I'd land #1 before enabling scanners in the visual flow (it writes malformed YAML) and #2 is a small parity fix that removes a dead-end in the canvas. |
|
@claude conduct an adversarial review using this skill: https://claude.ai/customize/skills?selectedId=skill_01U57srusYSMerLEneHv2hgM
|
|
Claude finished @eblairmckee's task in 9m 28s —— View job Adversarial Review — RPCN schema/YAML utils + feature-flag safety
✅ Prior review findings — all confirmed fixedVerified against current code: 🟠 New — Correctness (reachable with the flag OFF)1. const processors = (processorsNode?.toJSON?.() as unknown[]) || []; // strips AST → plain JS, no comments/anchors
...
doc.setIn(['pipeline', 'processors'], [...processors, newProcessor]); // rebuilds array from stripped valuesReading the existing array through 2. const existing = (node?.toJSON?.() as unknown[]) || [];
...
doc.setIn([key], [...existing, newResource]);Identical defect for 3. Picking a scanner when the config has no 🟡 Minor4. Output empty-case labels reuse the raw YAML index — the "no numbering gaps" fix from the processor path wasn't mirrored — 5. 6. Feature-flag safety (requested focus #2) — sound, with two test gapsThe flag-OFF experience is correct. Visual lanes render only when
Non-flagged behavior changes worth noting:
Bottom line: flag-OFF is safe to ship. Before enabling scanners/component-insertion widely I'd land #1–#3 (all silently produce degraded/invalid YAML on the shared, non-flagged merge path) and add append-into-existing-array merge tests. #4–#6 are minor. |
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), edit a switch case's routing condition inline, 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) — 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; aliases live inconnect-command-palette-utils.ts), 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