Skip to content

feat: pipeline page - view, edit, create modes with pipeline flow diagram#2285

Open
eblairmckee wants to merge 5 commits intomasterfrom
pr4/edit-create-mode-pipeline-page
Open

feat: pipeline page - view, edit, create modes with pipeline flow diagram#2285
eblairmckee wants to merge 5 commits intomasterfrom
pr4/edit-create-mode-pipeline-page

Conversation

@eblairmckee
Copy link
Contributor

@eblairmckee eblairmckee commented Mar 11, 2026

Summary

  • Redesigns the pipeline page with an IDE-like resizable panel layout across all three modes (view, edit, create)
  • View mode: read-only details panel (pipeline info, throughput chart, tags, secrets/topics references), logs panel, and flow diagram sidebar
  • Edit/create mode: YAML editor with lint hints, config dialog (name, description, compute units, tags), command palette (Cmd+Shift+P), connector picker, and mini wizard for Redpanda components
  • Flow diagram: renders PipelineFlowDiagram in the left sidebar for all modes, gated behind enablePipelineDiagrams feature flag
  • Bug fix: serverless init guard — useEffect now early-returns when components.length === 0 and includes components in deps, fixing a race where getConnectTemplate silently failed before component list resolved

View

Screenshot 2026-03-11 at 7 06 27 PM

Edit

Screenshot 2026-03-11 at 7 06 33 PM

Create (empty)

Screenshot 2026-03-11 at 7 06 44 PM

Create (wizard completed)

Screenshot 2026-03-11 at 7 07 08 PM

PR #2284 review comments addressed

  • Extracted throughput chart helpers to utils/pipeline-throughput.utils.ts with 12 unit tests
  • Removed 5 unnecessary useMemo/useCallback in throughput card
  • Regenerated yarn.lock

Test coverage

  • 17 unit tests: 12 throughput utils (series merging, formatting) + 5 constants (isSystemTag, isCloudManagedTagKey)
  • 20 integration tests: 9 config component (name input, description, compute slider, tags CRUD) + 11 view details (pipeline info, compute units, URL, service account, tags, secrets, topics, delete)

Feature flags

  • enablePipelineDiagrams — gates PipelineFlowDiagram in the left sidebar (all modes)

Reviewer actions

  1. Verify add-secrets-dialog.tsx signature change (onSecretsCreated: (secretNames?: string[]) => void) doesn't break callers in onboarding-wizard.tsx and api-connect-wizard.tsx — param is optional so existing callers should work
  2. Verify onboarding wizard still works end-to-end after Card wrapper removal at CREATE_CONFIG step — PipelinePage now owns its own layout
  3. Test edit mode: open an existing pipeline → verify resizable panel layout, editable name in toolbar, config dialog (Settings button), command menu (Cmd+Shift+P), connectors card in left sidebar
  4. Test create mode via wizard: reach final step → verify same layout as edit, form fields persist across wizard navigation
  5. Test view mode: open a pipeline in view → verify details panel, throughput card, logs panel, toolbar status badge + start/stop + edit
  6. With enablePipelineDiagrams on: verify flow diagram renders in left sidebar for all three modes

@secpanda
Copy link

secpanda commented Mar 11, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

isLoading,
nameError,
autoFocus,
// biome-ignore lint/complexity/noExcessiveCognitiveComplexity: Toolbar renders multiple conditional UI states
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: if you do find any time, please try and avoid the complexity being ignored, I think the LLM starts to notice this pattern more and more so it would just slam another biome-ignore and move on

@malinskibeniamin
Copy link
Contributor

@claude review

@claude
Copy link

claude bot commented Mar 11, 2026

Claude finished @malinskibeniamin's task in 2m 32s —— View job


PR Review: Edit/Create Mode Pipeline Page

  • Gather context and understand the PR
  • Read key changed files
  • Review architecture and patterns
  • Check for bugs, security, and correctness issues
  • Review test coverage

Overall the PR is well-structured with good separation of concerns. The orchestration in index.tsx is intentionally complex and the biome-ignore is justified. Below are findings ranging from bugs to nits.

Bugs

1. Duplicate entry in PRIORITY_COMPONENTSconnect-tiles.tsx:40,44

mysql_cdc appears twice in the PRIORITY_COMPONENTS array. The second occurrence (index 12) is a no-op since the sort deduplication logic uses PRIORITY_COMPONENTS.includes(), but it's still dead/confusing:

const PRIORITY_COMPONENTS = [
  // ...
  'mysql_cdc',      // index 7
  'mongodb_cdc',
  'snowflake_streaming',
  'redpanda_migrator',
  'mysql_cdc',      // index 12 - duplicate
  'snowflake_put',

Fix this →

2. Conflicting SVG gradient IDs — pipeline-throughput-card.tsx:163-170 and chart-skeleton.tsx:97-104

Both components use hardcoded gradient IDs (fillIngress/fillEgress in throughput card; skelFill1/skelFill2 in chart skeleton). If two instances ever render on the same page (e.g., in a list view or Storybook), the gradients will silently collide — the second card will paint with the first card's gradient colors.

Consider generating unique IDs with useId() from React.
Fix this →

3. crypto.randomUUID() as React key — connect-tiles.tsx:135

Skeleton tiles use crypto.randomUUID() directly as the key prop, which regenerates on every render and defeats React's reconciliation entirely:

const skeletonTiles = Array.from({ length: tileCount }, () => (
  <div ... key={crypto.randomUUID()}>   // new key every render

Replace with index-based keys or a stable pre-generated list.
Fix this →

Correctness / Potential Issues

4. Render-body ref mutation — index.tsx:201-205

The pattern of comparing and resetting a ref directly in the render body is fragile with React Strict Mode / Concurrent Features — the render function may run multiple times before committing:

// These lines run during render, not in an effect
const prevYamlRef = useRef(yamlContent);
if (prevYamlRef.current !== yamlContent) {
  prevYamlRef.current = yamlContent;
  userLintOverrideRef.current = null;  // side effect in render
}

The intent (reset manual lint panel override when YAML changes) is better served by moving this into the handleYamlChange callback directly, which already has a reference to userLintOverrideRef.

5. existingSecrets={[]} always empty — index.tsx:692

The AddSecretsDialog is passed existingSecrets={[]} unconditionally, so QuickAddSecrets never knows about secrets already referenced in the YAML. This means users could create duplicate secrets without any warning. Consider deriving existing secrets from yamlContent (you already have extractSecretReferences used in view-details.tsx).

6. useListSecretsQuery is always active — pipeline-command-menu.tsx:83

Unlike useListUsersQuery (which correctly gates on { enabled: open }), the secrets query runs unconditionally regardless of whether the command menu is open. This causes an unnecessary network request on page load.

7. form.watch('name') in JSX — index.tsx:579

form.watch('name') subscribes to all form re-renders. Given the name is typed character-by-character and drives the toolbar, this will cause the entire PipelinePage to re-render on every keystroke. Consider useWatch({ control, name: 'name' }) which is more explicit, or lift the name state out of the form for the toolbar.

Nits / Style

8. MIN_TASKS re-declared in test — config.test.tsx:22

The test file re-declares const MIN_TASKS = 1 instead of importing from source. If the minimum changes, the test silently continues to pass:

// config.test.tsx
const MIN_TASKS = 1;  // should import from '../tasks'

9. Brittle className assertion in test — config.test.tsx:139-149

Testing expect(keyInput.className).toContain('bg-primary-alpha-default') couples the test to an implementation-detail Tailwind class. If the styling changes (e.g., different shade), the test breaks without any behavior change. Consider testing visible behavior instead.

10. useListTopicsQuery({ pageSize: 500 }) hardcoded — pipeline-command-menu.tsx:89

500 is likely sufficient for most clusters, but worth a comment explaining why this cap was chosen, or wiring it to a constant.

11. hasInput / hasOutput detection is fragile — index.tsx:588-591

yamlContent.includes('input:') will also match input: appearing inside comments or string values. This is low-risk in practice, but consider using the existing YAML parser utilities instead.

12. lint-hint-list.tsx indentation inconsistency

Lines 44, 46-48 have mixed indentation and inline className on SimpleCodeBlock that sits outside the component's normal style. The reviewer's note about adding syntax highlighting support is acknowledged.

Addressing Reviewer Comments

  • chart-skeleton.tsx / recharts v2 vs v3: The reviewer's question is valid. Since shadcn is still holding on recharts v2, continuing with v2 here avoids a potentially breaking migration and is the right call for now. Worth tracking the upstream shadcn issue.
  • editable-text.tsx: The reviewer asks if this should live in the UI registry or under typography. Given it's a fairly general-purpose editable input that matches heading styles, it's a good candidate for the registry — especially since it would benefit from consistent behavior across the app.
  • Icon imports (lint-hint-list.tsx:15, toolbar.tsx:42): Both lucide-react imports should go through components/icons per the reviewer's notes for consistency.
  • toolbar.tsx:201 biome-ignore: The noExcessiveCognitiveComplexity suppression is added to a component that was already complex. The reviewer is right that this pattern signals the need for decomposition — the renderActionButton logic and the state/config factories are already a step in the right direction, but could be split into a PipelineActionButton sub-component.

Summary

The PR introduces a well-thought-out IDE-like layout with clean state management and good test coverage for the Config form. The main actionable items before merge are:

  1. Bug: Deduplicate mysql_cdc in PRIORITY_COMPONENTS
  2. Bug: Fix crypto.randomUUID() skeleton keys
  3. Bug: Scope SVG gradient IDs with useId()
  4. Cleanup: Move render-body side effect (userLintOverrideRef.current = null) into handleYamlChange
  5. Correctness: Gate secrets query on open in command menu

@eblairmckee eblairmckee changed the title Pr4/edit create mode pipeline page feat: pipeline page - view, edit, create modes with pipeline flow diagram Mar 12, 2026
@eblairmckee
Copy link
Contributor Author

@claude

@claude
Copy link

claude bot commented Mar 12, 2026

Claude finished @eblairmckee's task in 24s —— View job


Hey @eblairmckee! It looks like your comment just has @claude with no additional instructions. Could you clarify what you'd like me to do?

For example:

  • Implement the bug fixes from my previous review (duplicate mysql_cdc, crypto.randomUUID() keys, SVG gradient IDs, secrets query gating, etc.)
  • Answer a specific question about the PR
  • Something else entirely

Just let me know and I'll get started!

@eblairmckee
Copy link
Contributor Author

@claude review this PR

@claude
Copy link

claude bot commented Mar 12, 2026

Claude finished @eblairmckee's task in 1m 59s —— View job


PR Review: Pipeline Page - View, Edit, Create Modes

  • Gather context and understand the PR
  • Read key changed files
  • Review architecture and patterns
  • Check for bugs, security, and correctness issues
  • Review test coverage

Good news first: the bugs flagged in the previous review (@malinskibeniamin's round) have all been addressed in the current state of this branch:

  • ✅ Duplicate mysql_cdc in PRIORITY_COMPONENTS — gone
  • crypto.randomUUID() skeleton keys — now uses index-based (_, i) with key={i}
  • ✅ SVG gradient ID collisions — both AreaVariant (chart-skeleton.tsx) and pipeline-throughput-card.tsx correctly use useId()
  • ✅ Secrets query gated on openuseListSecretsQuery({}, { enabled: open }) at line 83
  • useWatch instead of form.watchuseWatch({ control: form.control, name: 'name' }) at line 559

Below are the remaining issues I found.

Bugs / Correctness

1. useListTopicsQuery not gated on openpipeline-command-menu.tsx:90

Unlike secrets/users queries which correctly use { enabled: open }, the topics query fires unconditionally on every page load regardless of whether the command menu has ever been opened:

// secrets — correctly gated ✅
const { data: secretsResponse } = useListSecretsQuery({}, { enabled: open });

// topics — always fires ❌
const { data: topicsResponse } = useListTopicsQuery({ pageSize: 500 });

Fix this →

2. existingSecrets={[]} always empty — index.tsx:692

AddSecretsDialog is passed a hardcoded empty array. The component uses existingSecrets to warn about duplicates, so users can create colliding secrets without any feedback. The YAML secrets are already extracted elsewhere — extractSecretReferences/getUniqueSecretNames are imported in view-details.tsx and could be reused here.

Fix this →

3. Fragile hasInput/hasOutput detection — index.tsx:588-591

String .includes('input:') and .includes('output:') will match occurrences inside YAML comments (# no input: here) or string values. Low-risk today but can produce false positives that hide the sidebar card incorrectly. The YAML parser is already available in this file via parseSchema/getConnectTemplate.

Nits / Code style

4. Lucide icons imported directly — config.tsx:26, pipeline-command-menu.tsx:23, connect-tiles.tsx:20-21

Several new files import directly from lucide-react rather than going through components/icons, which the reviewer flagged consistently (and toolbar.tsx already complies):

// config.tsx
import { PlusIcon, XIcon } from 'lucide-react'; // should be components/icons

// pipeline-command-menu.tsx
import { PlusIcon } from 'lucide-react'; // same

// connect-tiles.tsx
import { Search } from 'lucide-react'; // same

5. ref={null} in standalone dialogs — index.tsx:710, index.tsx:724

AddTopicStep and AddUserStep are forwardRef components. Passing ref={null} explicitly (rather than omitting the prop) works but is misleading — it suggests the ref is intentionally suppressed rather than simply not needed. Since these are standalone dialogs without a wizard flow, the ref can just be omitted.

6. EditableText autoFocus initializes state — editable-text.tsx:51

const [isEditing, setIsEditing] = useState(autoFocus ?? false);

This works correctly for initial render, but if autoFocus changes after mount (e.g. switching from create → edit mode on the same component instance), the component won't respond. This is unlikely in practice given the current routing, but worth noting for future-proofing.

Test coverage

7. config.test.tsx — Redundant final test (lines 138-151)

'tag key input is rendered and editable' is almost identical to 'tag key and value inputs are editable' (lines 121-136) — both type 'test' into the key input and assert the value. The test suite would be cleaner with this test removed or replaced with validation-specific coverage (e.g., verify the error message appears when a key is empty).

8. config.test.tsx — Test schema diverges from source

The test defines its own inline Zod schema (line 23) with a different tags.value rule (.min(0) vs .min(1, 'Value is required') in production). If the source schema validation changes, tests may not catch regressions. Consider importing the schema and form defaults from the production module.

Summary

The architecture is clean and well-structured. The previous review's bug fixes are all in. The main items worth addressing before merge:

  1. Gate useListTopicsQuery on open in pipeline-command-menu.tsx (unnecessary network call)
  2. Populate existingSecrets in AddSecretsDialog from YAML parse (otherwise duplicate-secret guard is inert)
  3. Icon imports through components/icons in 3 new files
  4. Test cleanup — remove redundant final test in config.test.tsx

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a nice addition to backport to registry afterwards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants