feat(deployments): support multi-version flows#12950
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (49.97%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## release-1.10.0 #12950 +/- ##
==================================================
+ Coverage 53.90% 54.06% +0.15%
==================================================
Files 2051 2060 +9
Lines 187497 188372 +875
Branches 26653 26827 +174
==================================================
+ Hits 101072 101842 +770
- Misses 85306 85411 +105
Partials 1119 1119
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ment-multi-flow-versions # Conflicts: # src/frontend/src/pages/MainPage/pages/deploymentsPage/components/step-attach-flows-version-panel.tsx # src/frontend/src/pages/MainPage/pages/deploymentsPage/components/step-attach-flows.tsx
Cristhianzl
left a comment
There was a problem hiding this comment.
🎯 Executive Summary
Functionally strong PR. Enables attaching multiple versions of the same flow to a deployment, with flowId:versionId scoped keys, undo of detach in edit mode, stable default tool names, and a clean refactor of the Review step into smaller components.
| Category | Status | Notes |
|---|---|---|
| 🔴 CRITICAL — Security / PII | ✅ OK | No PII in logs, no secrets in code, no console.* |
| 🔴 CRITICAL — DRY | "Scoped fallback" logic duplicated in 4 places | |
| 🔴 CRITICAL — File Structure | deployment-stepper-context.tsx = 814 lines (limit 500–700) |
|
| 🟠 IMPORTANT — SOLID | Context SRP; overloaded signature in handleSelectVersion |
|
| 🟠 IMPORTANT — Errors / Typing | ✅ OK | Strong typing, no any, errors handled |
| 🟡 RECOMMENDED — Observability | ✅ OK | No logs; modal surfaces errors via toast |
| 🟢 TESTING | ✅ OK | Happy + adversarial paths covered |
🔴 CRITICAL
1. deployment-stepper-context.tsx exceeds the line limit
File: src/frontend/src/pages/MainPage/pages/deploymentsPage/contexts/deployment-stepper-context.tsx (814 lines)
Rule violated: Hard limit: 500 lines (up to 600–700 only if all other rules pass) — REVIEWER_RULE.md §"File Structure Limits"
The Provider mixes very distinct responsibilities in the same file:
| Responsibility | Functions | Prefix | Approx. lines |
|---|---|---|---|
| Normalization / key utility | normalizeSelectedFlowVersions, getScopedValue, getScopedToolName |
normalize* / get* |
134–183 |
| Payload building (business rule orchestration) | buildProviderAccountPayload, buildConnectionPayloads, buildDeploymentPayload, buildDeploymentUpdatePayload |
build* |
419–705 |
| Stepper state / handlers | handleNext, handleBack, handleSelectVersion, handleRemoveAttachedFlow, handleUndoRemoveFlow |
handle* / set* |
297–416 |
React Provider + final useMemo |
DeploymentStepperProvider |
— | 185–804 |
Why this matters:
- Mixing prefixes (
normalize*+build*+handle*+get*) violates "Mandatory Separation by Responsibility" (REVIEWER_RULE.md §246-258). - The
build*functions are pure domain logic (state → API payload) — they don't depend on React and could be tested without the Provider. - The Provider should orchestrate; today it is the entire module.
How to fix:
contexts/
└── deployment-stepper-context.tsx # Provider + state (≤300 lines)
helpers/
├── version-scope.ts # normalizeSelectedFlowVersions, getScopedValue, getScopedToolName
└── deployment-payload-builders.ts # build*Payload (pure functions)
The build* functions today close over React state; exposed as pure functions they would receive state as arguments and become trivially testable in isolation (today the tests in deployment-stepper-payload-builders.test.tsx need to mount the Provider and use act() to test them — a clear coupling smell).
💡 Estimate: after the split, the Provider file should drop to ~300 lines and
build*Payloadgain cheap, isolated test coverage.
2. DRY — "Scoped key fallback" logic duplicated in 4 places
Rule violated: REVIEWER_RULE.md §"DRY Principle" — "5+ lines appearing 2+ times = extract"
The rule "look up by attachmentKey; if missing and only one version exists for that flowId, fall back to flowId" is replicated:
| Location | Function | Lines |
|---|---|---|
contexts/deployment-stepper-context.tsx:155-161 |
getScopedValue |
7 |
contexts/deployment-stepper-context.tsx:163-183 |
getScopedToolName |
21 |
components/step-review/utils.ts:9-28 |
getToolNameForReview |
20 |
components/step-review/utils.ts:30-49 |
getInitialToolNameForReview |
20 |
getToolNameForReview (utils.ts:9) is essentially identical to getScopedToolName (context.tsx:163) — only the versionMap shape differs (Map vs list). Same business rule, rewritten.
How to fix:
Extract into helpers/version-scope.ts:
export function getScopedValue<T>(
map: Map<string, T>,
attachmentKey: string,
flowId: string,
): T | undefined { ... }
export function getScopedValueWithUniqueVersion<T>(
map: Map<string, T>,
attachmentKey: string,
flowId: string,
flowVersionCount: number,
): T | undefined { ... }Reuse in both the context and step-review/utils.ts.
🟠 IMPORTANT
3. SRP — Overloaded signature in handleSelectVersion
File: contexts/deployment-stepper-context.tsx:93-98 and :388-414
handleSelectVersion: (
flowId: string,
flowNameOrVersionId: string, // 👈 meaning shifts
versionIdOrVersionTag: string, // 👈 meaning shifts
versionTag?: string,
) => void;The semantics of arguments 2 and 3 depend on whether the 4th is provided. This is "do-stuff-with-conditional-meaning" — the same anti-pattern described in REVIEWER_RULE.md §"SRP — How to Spot Violations" (processOrder(), handleEverything()).
Real call sites:
step-attach-flows.tsx:127-134: passes 4 arguments (the "complete" path)- 3-argument calls also possible (legacy callers without
flowName).
How to fix:
Lock down a single object signature:
handleSelectVersion: (params: {
flowId: string;
flowName: string;
versionId: string;
versionTag: string;
}) => void;If a legacy caller has no flowName, it explicitly passes "Flow" — visible at the call site.
4. SRP / KISS — step-review/utils.ts mixes prefixes
File: components/step-review/utils.ts (255 lines)
Coexisting prefixes: normalize* (validation/format), build* (formatting), get* (data lookup). Per REVIEWER_RULE.md §246-258, these prefixes belong in separate files.
The mitigation here is reasonable (all are pure and review-specific), but:
normalizeWxoName(utils.ts:5-7) is provider-specific ("Wxo" = watsonx Orchestrate). A similar utility likely already exists — verify withgrep -rn "wxo\|watsonx" src/frontend --include="*.ts". If it doesn't, move it tohelpers/wxo-name.tsto make scope explicit.getToolNameForReview/getInitialToolNameForRevieware lookups (item #2 above): move to the shared helper.- What remains (
buildReviewFlows,buildToolNamesToCheck,buildToolNameErrors) is cohesive — keep.
5. Magic strings / inconsistent fallbacks
Locations:
| File:line | Value | Suggestion |
|---|---|---|
step-review.tsx:84 |
"Unknown flow" |
constant UNKNOWN_FLOW_LABEL |
step-review/utils.ts:119 |
"Unknown" |
same constant |
step-attach-flows.tsx:280 |
"Flow" |
constant DEFAULT_FLOW_LABEL |
deployment-stepper-modal.tsx:108 |
"Flow" |
same constant |
contexts/deployment-stepper-context.tsx:395, 496, 585 |
"Flow" |
same constant |
contexts/deployment-stepper-context.tsx:424 |
"watsonx-orchestrate" |
already exists? yes, in mockInstance.provider_key (tests) and mappers — confirm whether WXO_PROVIDER_KEY is already defined |
Three different fallbacks (
"Unknown flow","Unknown","Flow") represent the same concept ("could not resolve flow name"). Standardize.
6. Redundant as string cast
File: deployment-stepper-modal.tsx:124-127
const llm =
typeof deploymentDetail?.provider_data?.llm === "string"
? (deploymentDetail.provider_data.llm as string) // 👈 redundant
: "";The typeof === "string" check already narrows the type. The cast is dead weight and hides the real type of provider_data.llm (likely unknown in the schema). Drop the cast and, ideally, type provider_data properly in Deployment (types.ts:95) — today it's Record<string, unknown>, which forces this pattern.
🟡 RECOMMENDED
7. The hydration useEffect in the context could be a useState initializer
File: contexts/deployment-stepper-context.tsx:276-295
The effect exists to populate state when initialState arrives asynchronously (after the attachments fetch resolves). Today:
- Provider mounts with empty Maps.
initialStatechanges when the fetch resolves.- Effect checks
hasHydratedEditStateRefand populates state.
But deployment-stepper-modal.tsx:148-149 already uses key={...editingDeployment?.id...} to remount the Provider when the deployment changes. Combined, you can resolve this with useState(() => initialState ?? defaults) in the Provider and drop the effect — simplifies the logic and removes the hasHydratedEditStateRef.
Why it matters: the effect runs on every render until the guards short-circuit; in production it's cheap, but it widens the surface for subtle race bugs (between setState calls).
8. isAttachedVersion has 3 redundant code paths
File: step-attach-flows-version-panel.tsx:81-89
const isAttachedVersion =
selectedVersionByFlow.has(attachmentKey) ||
selectedVersionByFlow.get(selectedFlow.id)?.versionId === version.id ||
Array.from(selectedVersionByFlow.values()).some(
(entry) =>
entry.flowId === selectedFlow.id &&
entry.versionId === version.id,
);3 OR-checks for the same question:
- Has the canonical key?
- Has the legacy key (
flowId) and the versionId matches? - Any entry with these ids? (O(n) scan)
After normalizeSelectedFlowVersions, every key should be in canonical form — cases (2) and (3) only matter if non-normalized keys leak from state. Confirm and simplify to selectedVersionByFlow.has(attachmentKey). If the triple check is "defensive" for legacy data, it should be temporary and tracked by a ticket.
9. Naming — getScopedValue is too generic
File: contexts/deployment-stepper-context.tsx:155
getScopedValue doesn't reveal intent (REVIEWER_RULE.md §"Naming"). getValueByAttachmentKeyOrFlowId or getValueByScopedKey makes the fallback explicit.
10. // biome-ignore + // eslint-disable-next-line in step-attach-flows.tsx:202-204
File: step-attach-flows.tsx:202-257
Both ignores are present ("intentionally run only on mount"), but the effect has real dependencies (flows, initialFlowId, selectedVersionByFlow, detectEnvVars, defaultToolNameScopeId, setErrorData, setToolNameByFlow, updateDetectedEnvVars). The guard is handledPreselectedAttachmentRef.current === preSelected.key — valid, but the presence of two suppression comments hints at friction: either deps truly almost never change (and it's fine) or the effect should be refactored into a useMemo + minimal effect. Worth reviewing.
🟢 TESTING
✅ Strengths:
edit-mode.test.tsx: covers happy path (multi-step edit) and adversarial ("blocks update flow when existing name does not start with a letter" — line 111).deployment-stepper-payload-builders.test.tsx: coversbuildConnectionPayloads,isNewfiltering, deduplication.step-attach-flows.test.tsx: tests real panel interaction (clean mocks, no leakage between tests).step-review.test.tsx: covers tool name validation, duplicates, errors.
- Tests for
buildPayload*run inside the Provider viarenderHook. After the split suggested in #1, those tests would become pure (describe('buildDeploymentPayload', () => { ... })calling a pure function). Faster suite, no React lifecycle coupling. - I didn't see a regression test for "preserve removed flow state" (commit cbae4ff) — confirm there's a scenario covering "user clicks detach, navigates to another step, comes back — state preserved".
🔒 Security / PII / Integrations
✅ All clean:
- No
console.*, noprint()in modified files. - No hardcoded secrets — credentials (
api_key,url) are collected via form input and sent through a mutation. - No
eval/exec/dangerouslySetInnerHTML. - No
: anyor: object. - No user data in error messages (only generic toast titles).
- No TODOs without ticket references.
- No webhook/HMAC integration in the diff — N/A.
- No AI/LLM logic in the diff — N/A.
📐 Architecture
✅ Good:
- Clean separation of presentational components (
FlowListPanel,VersionPanel,ReviewSummaryCard,ReviewFlowConfigCard,ReviewDetachingSection,EditableToolName) — allmemo-ed where appropriate. - Custom
useConnectionPanelStatehook properly extracted. - The comment block in
deployment-stepper-modal.tsx:84-95explaining the tool-name caching contract (Langflow ↔ wxO) is a perfect example of a "WHY comment", not a "WHAT comment".
- The Provider remains the feature's "god object" — see #1.
📊 Final Score
| Category | Weight | Status | Findings |
|---|---|---|---|
| 🔴 PII/Security | Blocker | ✅ | Pass |
| 🔴 DRY | Blocker | #2 | |
| 🔴 File Structure | Blocker | #1 | |
| 🟠 SRP/SOLID | High | #3, #4 | |
| 🟠 Naming/Magic strings | High | #5, #9 | |
| 🟠 Error Handling | High | ✅ | Pass |
| 🟠 Strong Typing | High | ✅ | Pass (with #6 polish) |
| 🟡 Observability | Medium | ✅ | Pass |
| 🟢 Testing | Low | ✅ | Pass (with post-split suggestion) |
✅ Action checklist for the author
Blockers (resolve before merge):
- #1 Split
deployment-stepper-context.tsxintohelpers/version-scope.ts+helpers/deployment-payload-builders.ts+ Provider. - #2 Extract
getScopedToolName/getScopedValueand the equivalents instep-review/utils.tsinto a single shared helper.
Important (preferably this PR):
- #3 Replace the overloaded
handleSelectVersionsignature with a typed object. - #5 Standardize
"Unknown flow"/"Unknown"/"Flow"into a single constant. - #6 Drop the redundant
as stringcast; typeprovider_data.llm.
Recommended (can ship as a follow-up PR):
- #4 Move
normalizeWxoNameto a dedicated file. - #7 Consider replacing the hydration
useEffectwith auseStateinitializer. - #8 Simplify the triple
isAttachedVersioncheck (assuming the normalizer guarantees the invariant). - #9 Rename
getScopedValueto something more descriptive. - #10 Consider refactoring the effect at
step-attach-flows.tsx:204to remove the two suppressions.
Test suggestions post-split (#1):
- After moving
build*Payloadto a pure module, convertdeployment-stepper-payload-builders.test.tsxto pure tests (norenderHook). - Add an explicit regression case for "preserve removed flow state across step navigation".
… tool names
- use-connection-panel-state.test.ts: rename effectiveFlowId → effectiveAttachmentKey
in baseParams() to match hook's refactored param name
- deployment-create.spec.ts: make SNAPSHOTS_DUPLICATE_MOCK route dynamic — echo back
requested names as existing tools so duplicate check works with scoped names
(getDefaultDeploymentToolName now generates "Flow {scope}-{id}" format)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Behavior Changes
Notes
Screen.Recording.2026-04-30.at.3.08.57.PM.mov