feat: pipeline page - view, edit, create modes with pipeline flow diagram#2285
feat: pipeline page - view, edit, create modes with pipeline flow diagram#2285eblairmckee merged 39 commits intomasterfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 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 |
There was a problem hiding this comment.
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
|
@claude review |
|
Claude finished @malinskibeniamin's task in 2m 32s —— View job PR Review: Edit/Create Mode Pipeline Page
Overall the PR is well-structured with good separation of concerns. The orchestration in Bugs1. Duplicate entry in
const PRIORITY_COMPONENTS = [
// ...
'mysql_cdc', // index 7
'mongodb_cdc',
'snowflake_streaming',
'redpanda_migrator',
'mysql_cdc', // index 12 - duplicate
'snowflake_put',2. Conflicting SVG gradient IDs — Both components use hardcoded gradient IDs ( Consider generating unique IDs with 3. Skeleton tiles use const skeletonTiles = Array.from({ length: tileCount }, () => (
<div ... key={crypto.randomUUID()}> // new key every renderReplace with index-based keys or a stable pre-generated list. Correctness / Potential Issues4. Render-body ref mutation — 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 5. The 6. Unlike 7.
Nits / Style8. The test file re-declares // config.test.tsx
const MIN_TASKS = 1; // should import from '../tasks'9. Brittle className assertion in test — Testing 10. 500 is likely sufficient for most clusters, but worth a comment explaining why this cap was chosen, or wiring it to a constant. 11.
12. Lines 44, 46-48 have mixed indentation and inline Addressing Reviewer Comments
SummaryThe 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:
|
eblairmckee
changed the title
|
Claude finished @eblairmckee's task in 24s —— View job Hey @eblairmckee! It looks like your comment just has For example:
Just let me know and I'll get started! |
|
@claude review this PR |
|
Claude finished @eblairmckee's task in 1m 59s —— View job PR Review: Pipeline Page - View, Edit, Create Modes
Good news first: the bugs flagged in the previous review (@malinskibeniamin's round) have all been addressed in the current state of this branch:
Below are the remaining issues I found. Bugs / Correctness1. Unlike secrets/users queries which correctly use // secrets — correctly gated ✅
const { data: secretsResponse } = useListSecretsQuery({}, { enabled: open });
// topics — always fires ❌
const { data: topicsResponse } = useListTopicsQuery({ pageSize: 500 });2.
3. Fragile String Nits / Code style4. Lucide icons imported directly — Several new files import directly from // 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'; // same5.
6. const [isEditing, setIsEditing] = useState(autoFocus ?? false);This works correctly for initial render, but if Test coverage7.
8. The test defines its own inline Zod schema (line 23) with a different SummaryThe architecture is clean and well-structured. The previous review's bug fixes are all in. The main items worth addressing before merge:
|
There was a problem hiding this comment.
This could be a nice addition to backport to registry afterwards
… z index issue for slash command menu
…iew table as well as pipeline config form
|
Claude finished @eblairmckee's task in 3m 22s —— View job Review: Changes Since March 12th 2026 (Post-Approval)
Good work on the feature-flag layering strategy — the three-tier rollback plan is well thought out. Below are findings specific to the post-approval work (March 18–19). Summary of post-approval changes
Bugs / Regressions1. Feature flag name mismatch — The PR description (eblairmckee's March 19 comment) names the flag // constants.ts
enableConnectSlashMenu: false,
// index.tsx
const isSlashMenuEnabled = isFeatureFlagEnabled('enableConnectSlashMenu');If Cloud UI injects flags by name, a mismatch here means the feature silently stays off. Worth confirming the name matches the flag key in Cloud UI config. Fix this → 2. Secrets query lost its The March 12th review confirmed secrets was gated ( // ❌ now fires on every page load
const { data: secretsResponse } = useListSecretsQuery({});
// ❌ still not gated (flagged in March 12 review)
const { data: topicsResponse } = useListTopicsQuery({ pageSize: 500 });
// ✅ still correctly gated
const { data: usersResponse } = useListUsersQuery(undefined, { enabled: open });3. Slash trigger fires on URLs —
// 'http:/' → charBefore === ':' → TRIGGERS
// 'https:/' → sameUsers typing full URLs in YAML values (e.g. an HTTP input URL) will get an unexpected popover. Consider also excluding the case where the char before the previous slash is also a non-whitespace character, or restricting to start-of-value patterns. 4. Duplicate test files for same module — Both Correctness / Potential Issues5.
// Always mounted — fetches secrets + topics unconditionally
<PipelineCommandMenu ... open={isCommandMenuOpen} />
{isSlashMenuEnabled ? (
// Also mounted — same unconditional fetches
<PipelineCommandMenu ... variant="popover" open={slashCommand.isOpen} />
) : null}React Query deduplicates identical query keys so there won't be double network requests, but the unconditional fetching (issue #2 above) is amplified because fixing gating on one instance also needs to be applied correctly to both. The dialog variant's 6. The commit message says "hides header when either enableRpcnTiles or enablePipelineDiagrams are enabled", but the code only hides when // header.tsx:218-223
if (
isPipelineRoute &&
(isFeatureFlagEnabled('enablePipelineDiagrams') || (isFeatureFlagEnabled('enableRpcnTiles') && isEmbedded()))
) {
return true;
}In non-embedded OSS mode with only 7. The wizard's Nits8. Lucide icons still imported directly in import { PlusIcon } from 'lucide-react'; // should use components/iconsThis was flagged in the original review for other files and is still present in the new file. SummaryThe feature flag architecture is solid and the rollback story is clear. The main actionable items:
|
|
react doctor fixes in c2d847e
|
|
@malinskibeniamin when I updated data-table noticed an unintended breaking change that could've been upstreamed. fixed in commit: 4c43700 Breaking Change:
|
| Date | Who | Commit | Repo | What |
|---|---|---|---|---|
| Aug 12 2025 | Beniamin Malinski | 9c6620b3 |
ui-registry | Created data-table.tsx with simple { table } API |
| Mar 6 2026 | Beniamin Malinski | 7b92169bb |
console | Added DataTablePaginationState type + pagination prop to console's local copy of data-table.tsx. Updated all 8 call sites. Commit msg: "fix pagination issues on all list views" |
| Mar 13 2026 | Beniamin Malinski | 27d5e581b |
console | Added 'use no memo' directive to console's copy — belt-and-suspenders fix for more React Compiler issues in DataTable (PR #2297) |
| Mar 18 2026 | Blair McKee | f61dccaf7 |
console | "updated ui registry components" — overwrote local modifications with upstream, which never had them |
Root cause
Beniamin modified the registry component in-place in the console repo instead of either:
- Upstreaming the fix to the ui-registry, or
- Creating a local wrapper component that wouldn't be overwritten on sync
When Blair synced registry components, the upstream version (which never had the pagination prop) replaced the locally-modified version, breaking all 8 call sites.
Why the pagination prop existed
It was a React Compiler workaround. The commit 7b92169bb included this comment:
"Pass pagination primitives from the page so this footer still rerenders
when React Compiler keeps the table instance identity stable."
The problem: React Compiler aggressively memoizes, and TanStack Table reuses the same object reference for table across renders. The compiler saw a stable reference and skipped re-rendering the pagination footer — even when page index/size changed.
The workaround was to pass primitive values (pageIndex, pageSize, etc.) as a separate prop so React could detect changes via shallow comparison.
One week later (27d5e581b), 'use no memo' was added as a second workaround — opting the entire file out of React Compiler. This made the pagination prop redundant, but it was never removed.
Risk: React Compiler re-rendering regression
Both workarounds are now gone ('use no memo' directive + pagination prop). If the pagination stale-render bug resurfaces, the fix should be:
- Upstreamed to the registry, or
- Applied via a local wrapper component that won't be stomped by registry syncs
Do not modify registry files in-place — this is exactly how we got here.
What changed in the registry update
Old API (console-local modification):
<DataTablePagination
pagination={{
canNextPage: table.getCanNextPage(),
canPreviousPage: table.getCanPreviousPage(),
pageCount: table.getPageCount(),
pageIndex: pagination.pageIndex,
pageSize: pagination.pageSize,
}}
table={table}
/>New API (what the registry always had):
<DataTablePagination table={table} />Other changes in this update (non-breaking)
- Props interfaces now extend
SharedPropsinstead of definingtestIdinline DataTableFacetedFiltergained alabelClassNamepropDataTablePaginationStatetype deleted (never imported externally)- Various styling tweaks (button variants, badge variants, class ordering)
Affected files (8 call sites)
| File | Line |
|---|---|
src/components/pages/agents/list/ai-agent-list-page.tsx |
~484 |
src/components/pages/knowledgebase/details/knowledge-base-document-list.tsx |
~294 |
src/components/pages/knowledgebase/list/knowledge-base-list-page.tsx |
~559 |
src/components/pages/mcp-servers/list/remote-mcp-list-page.tsx |
~450 |
src/components/pages/rp-connect/pipeline/list.tsx |
~681 |
src/components/pages/secrets-store/secrets-store-list-page.tsx |
~416 |
src/components/pages/topics/Tab.Messages/index.tsx |
~1772 |
src/components/ui/connect/log-explorer.tsx |
~469 |
Fix
At each call site, replace the multi-line <DataTablePagination pagination={{...}} table={table} /> with <DataTablePagination table={table} />.
| @@ -0,0 +1,191 @@ | |||
| # useEffect Rules | |||
There was a problem hiding this comment.
I know we're adding a react-doctor rule for this, but this saved my sanity during this effort.
|
for transparency: Adversarial Review:
|
| Component | Tested | Gap |
|---|---|---|
config-dialog |
Basic rendering only | No save flow, no validation, no edit/view modes |
details-dialog |
Happy path rendering | No empty states, no delete flow, no overflow |
use-slash-command |
Good unit + integration | Missing multi-slash, rapid open/close, special chars |
pipeline-throughput-utils |
Good utility coverage | No error cases, no large dataset perf |
constants |
Basic coverage | No edge cases (empty string, mixed case) |
| pipeline/index.tsx | NONE | Zero test coverage for the main 700-line component |
| pipeline-command-menu.tsx | NONE | Zero test coverage for the 643-line command menu |
| connector-wizard.tsx | NONE | Zero test coverage for the 337-line wizard |
| toolbar.tsx | NONE | Zero test coverage |
The two largest components by line count (index.tsx and command-menu.tsx) have zero tests. This is the single biggest risk in the PR. Mode transitions, save flows, error handling, feature flag gating, and wizard integration are all untested.
Scope Creep Check
Items in this PR that are not directly related to "edit/create mode pipeline page":
- 6 shared UI component refactors (combobox, data-table, dropdown-menu, select, input, typography) — affects entire app
- Log explorer UX improvements — grid layout, live view prop, key removal
- DataTablePagination API simplification across 7 consumer pages — good cleanup, but cross-cutting
react-best-practices/rules/use-effect.md— documentation file, not code- AI gateway import cleanups — 3 files with minor import changes
- MCP server component tweaks — type changes in tool-card, tools-step, remote-mcp-configuration-tab
Verdict: ~15 files are tangential to the core feature. The shared UI changes are the most concerning because they create merge conflicts and make rollback complex.
Alternatives Considered
| Approach | Pros | Cons | Why rejected/accepted |
|---|---|---|---|
| Split shared UI changes into separate PR | Clean rollback, isolated testing | More PRs to manage | Recommended but not done |
| Feature flag everything | Safe rollout | Complexity | Done — good choice |
| Single monolithic component | Simpler file structure | 1000+ line component | Rejected — decomposed into dialogs/hooks |
| MobX for state | Existing pattern | Being removed from codebase | Rejected — uses React Query + Hook Form + Zustand correctly |
Summary
PLAN SUMMARY:
- Pipeline page: View/Edit/Create modes with proper routing and feature flag gating
- Command menu: Dual-mode (dialog + popover) for inserting variables/secrets/topics
- Shared UI: DataTablePagination simplified, portal container support added
- State: React Hook Form + Zustand + React Query layered approach
OUT OF SCOPE:
- Concurrent edit protection: Intentionally excluded, standard gap
- E2E tests: Not included in this branch
- Mobile/responsive: Not addressed
OPEN RISKS:
- Form+YAML desync (CRITICAL): queueMicrotask can cause stale YAML on save
- Shared UI blast radius (CRITICAL): 6 components modified, affects entire app
- Stuck error hints (HIGH): errorLintHints not cleared on new YAML edits
- Focus fighting (HIGH): Command menu retry loop is fragile
- Zero test coverage on main components (HIGH): index.tsx, command-menu.tsx untested
KNOWN UNKNOWNS:
- How does CloudV2 integration behave with the new getWizardConnectionData() fallback?
- What is the actual latency of metric queries for the throughput card at scale?
- Are there other consumers of the deleted details.tsx outside this repo?
Reviewer Verdict
Not ready to merge without addressing:
- The form+YAML desync race (Critical Add readiness & liveness check #1) — fix or add a guard
- The stuck error hints (High Create helm chart #3) — clear on YAML change
- Mutual exclusion for the two command menus (High Check list message performance (response time) #6)
Strongly recommended but not blocking:
4. Split shared UI component changes into a separate PR
5. Add at least smoke tests for index.tsx mode transitions and save flows
6. Zustand store cleanup in try/finally (High #5)
Acceptable as-is:
7. Everything marked MEDIUM and LOW
8. Test coverage gaps for utilities (they're covered at basic level)
9. New dependency (react-resizable-panels)
| return ( | ||
| <BadgeGroup | ||
| className="min-w-[184px]" | ||
| className="min-w-[174px]" |
There was a problem hiding this comment.
Seems oddly specific but all good
There was a problem hiding this comment.
I originally wanted to use flex-* values that were relative to available space, but it resulted in CLS when columns actually loaded, so opted to hard code them (purely eye balled what seemed right).
| if (isFeatureFlagEnabled('enablePipelineDiagrams') && isEmbedded()) { | ||
| navigate({ to: '/rp-connect/create' }); | ||
| } else { | ||
| navigate({ to: '/rp-connect/wizard', search: { step: undefined, serverless: undefined } }); |
There was a problem hiding this comment.
Shouldn't it be step 1?
There was a problem hiding this comment.
True, I guess it was implicit, as step 1 is default if no explicit step is provided.
| <ChartContainer className="h-40 w-full" config={chartConfig}> | ||
| <AreaChart data={chartData}> | ||
| <defs> | ||
| <linearGradient id={`${id}-ingress`} x1="0" x2="0" y1="0" y2="1"> |
There was a problem hiding this comment.
I think we can extract this to another component.
There was a problem hiding this comment.
How so? like a LinearGradient component? Or something else?
Commit e40e25c changesBug Fixes (all HIGH/MEDIUM issues from adversarial review addressed)
Test CoverageBefore: 77 tests across 4 files. Zero coverage on After: 145 tests across 10 files. New molecule-level integration tests (bottom-up)
Route-level tests (
|
…a components, decoupled persisted yaml state from new UX, fixed serverless intialization race condition, added link to component docs to leaves, added integration tests for rendered diagram
Throughput card improvements:Improved pipeline-throughput-card to be gated behind
|
…feature flag gating in pipeline-throughput-card
3 participants
Summary
PipelineFlowDiagramin the left sidebar for all modes, gated behindenablePipelineDiagramsfeature flag. Redpanda component leaves show setup-hint buttons ("+ Topic", "+ User") when topic/SASL config is missing, and doc links on hovergetConnectTemplate, inlined directly inPipelinePage. No separate wizard component — diagram hint buttons guide users to add topic/user config after insertionAddTopicStep/AddUserStep). On submit, surgically patches the YAML viatryPatchRedpandaYamlwithout regenerating the entire component blockuseCreateModeInitialYamlhook.useEffectearly-returns whencomponents.length === 0, readssessionStoragedirectly as hydration fallback, includes 3s timeout. Fixes race wheregetConnectTemplatesilently failed before component list resolvedenablePipelineDiagramsis active, YAML is no longer synced to the onboarding YAML store (the diagram UX doesn't use the wizard persistence path)connector-wizard.tsx,create-pipeline-sidebar.tsx,add-secrets-card.tsx,add-contextual-variables-card.tsxPR #2284 review comments addressed
utils/pipeline-throughput.utils.tswith unit testsuseMemo/useCallbackin throughput cardyarn.lockTest coverage (18 test files, ~225 test cases)
Onboarding steps (24 tests):
add-topic-step.test.tsx— topic creation/selection form, validation, error statesadd-user-step.test.tsx— SASL + service account auth flows, consumer group fieldsPipeline page (51 tests):
index.test.tsx(17) — create/edit/view mode rendering, form validation, save/cancel flows, serverless init skeleton, connector dialog integrationconfig-dialog.test.tsx(9) — name input, description, compute slider, tags CRUDdetails-dialog.test.tsx(11) — pipeline info, compute units, URL, service account, tags, secrets, topics, deletepipeline-command-menu.test.tsx(14) — slash command menu, variable insertion, keyboard navigationFlow diagram (30 tests):
pipeline-flow-diagram.test.tsx(15) — empty/error states, section headers, placeholder nodes, topic badges, setup hint buttons ("+ Topic" / "+ User"), docs linkspipeline-flow-nodes.test.ts(6) —getConnectorDocsUrlfor inputs/outputs/processors, non-docs sectionspipeline-flow-parser.test.tsx(9 new) —missingTopic/missingSaslflags on Redpanda components, SASL detection (component-level + rootredpanda.sasl), flag passthrough to layoutUtilities (109 tests):
yaml.test.tsx(24 new) —patchRedpandaConfig(topic/SASL patching),applyRedpandaSetup(surgical patch + template fallback),generateYamlFromWizardData,buildSaslPatch(service-account + SASL)use-create-mode-initial-yaml.test.tsx(12) — serverless path, non-serverless restoration, timeout, disabled state, diagrams-enabled gatinguse-slash-command.test.ts+.test.tsx(61) — slash trigger detection, cursor positioning, command insertion, popover lifecyclepipeline-throughput.utils.test.ts(12) — series merging, timestamp formatting, empty data handlingOther (15 tests):
constants.test.ts(5) —isSystemTag,isCloudManagedTagKeyonboarding-wizard-store.test.tsx(5) — store hydration,getWizardConnectionDatasessionStorage fallbackFeature flags
enablePipelineDiagrams— gatesPipelineFlowDiagramin the left sidebar (all modes), setup hint buttons, doc links, and YAML persistence decouplingenableConnectSlashMenu— gates inline/slash command menu in the YAML editorenableRpcnTiles— gates the tile-based connector picker in the onboarding wizardenableNewPipelineLogs— gatesLogExplorervs legacyLogsTabin view modeKnown issues
set-state-in-effectatindex.tsx:327—setYamlContent(pipeline.configYaml)in the form-reset effect triggers a React Compiler warning. Alternative approaches (queueMicrotask, derivation, key-remount) have worse tradeoffs. For now commenting ot react-doctor to ignore and suppress error.