refactor: unify the stepper machinery behind a shared provider and rebuild the deployment stepper on it - #14284
refactor: unify the stepper machinery behind a shared provider and rebuild the deployment stepper on it#14284tarciorodrigues wants to merge 7 commits into
Conversation
… stepperModal Promote modals/stepperModal into the canonical stepper framework (LE-1736 WP2 PR-a). The module so far only shipped the visual shell: the existing StepperContext merely echoes currentStep/totalSteps/title/description and every consumer hand-rolls its own transition state. This adds the missing generic layer, purely additive: - hooks/useStepperState.ts: headless transition logic — 1-based step, next/back/goTo with clamping, per-step canNext validation gate, minStep back floor (independent from initialStep), consumer-owned isBusy gating, onStepChange callback, reset, progressPercent using the indicators' (current-1)/(total-1) formula. Conditional steps are a computed steps list; the exposed step clamps if the list shrinks. - StepperProvider.tsx: context wrapper over the hook + useStepper(); shell-less flows (deploy-choice) can call the hook directly. - types.ts: StepperStepConfig / UseStepperStateOptions / StepperState / StepperProviderProps. - StepperModal.tsx: re-export the new API from the module entry. The API covers what the deployment stepper needs today (conditional step lists per mode, busy finish, back-disabled states, opening at a later step) and the deploy-choice phase machine (named steps, free goTo, auto-skip via initialStep with minStep 1), per the capability map in the ticket plan. StepperModal shell, StepperModalFooter and the knowledge base upload modal are untouched (module + consumer suites 125/125). Consumer migrations land in the next PRs of the sequence. Refs: LE-1736 W15
Unit suite for the new headless stepper framework (26 tests): initial state and clamping, next/back with per-step canNext gates (boolean and lazily re-evaluated function), minStep back floor vs free back after an auto-skip opening, goTo by id and by clamped number, conditional steps as a computed list (shrink clamps the exposed step, growing back restores it, navigation runs from the clamped step), busy gating of all transitions with reset still available, onStepChange firing only on committed transitions, progress formula including the single-step division-by-zero guard, and StepperProvider/useStepper context wiring. Refs: LE-1736 W16
…mitive Rebuild the deployment stepper's navigation machine on top of the stepperModal framework: the hand-rolled currentStep state, next/back clamps, edit-mode logical-step shifting and per-mode totalSteps in deployment-stepper-context are replaced by useStepperState with a computed step list (edit mode simply omits the provider step, so the getLogicalStep index shifting disappears). The public useDeploymentStepper() API is unchanged — currentStep, totalSteps, minStep, canGoNext, handleNext, handleBack keep their exact semantics: canGoNext stays the per-step domain gate (now keyed by step id instead of shifted numbers) and handleNext stays ungated with the same clamping (delegated to goTo), matching the setState machine it replaces. Step panels, modal, footer and indicator are untouched; all 31 deploymentsPage test suites (547 tests) pass without a single assertion change. Refs: LE-1736 W18
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughA reusable typed stepper state hook and context provider are added, exported, and covered by tests. Deployment navigation now uses the shared stepper state model with conditional edit-mode steps and existing validation gates. ChangesStepper state and deployment integration
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (7 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
refactor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/src/modals/stepperModal/hooks/useStepperState.ts`:
- Around line 42-46: Update commitStep in useStepperState so onStepChange is
invoked only after the corresponding state update is committed, preventing
batched next() calls from emitting duplicate transitions; preserve the no-op
behavior when from equals to, and add a regression test covering batched
transitions.
- Around line 8-10: Update clampStep to normalize minStep to an integer within
[1, totalSteps] before clamping, while retaining the zero-total-steps result of
0. Ensure back() cannot commit step 0, and validate or normalize fractional,
NaN, and other invalid numeric inputs from goTo and initial state so currentStep
is always a valid 1-based index.
In `@src/frontend/src/modals/stepperModal/StepperProvider.tsx`:
- Around line 13-23: Update StepperProvider and useStepperState so navigation
state is held in a provider-scoped Zustand store rather than local hook-managed
mutable state. Create or select the store per provider instance using the
existing key/instance mechanism, keep separate StepperProvider instances
isolated, and expose the selected store state through StepperStateContext
without changing the provider’s public behavior.
In
`@src/frontend/src/pages/MainPage/pages/deploymentsPage/contexts/deployment-stepper-context.tsx`:
- Around line 251-306: Guard handleNext with canGoNext before advancing the
stepper, so invalid state cannot move to the next step through the public
context. Preserve the existing stepper.goTo increment for valid transitions and
keep the current canGoNext predicates unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: edfa0cc3-2cae-4024-b33e-610285298599
📒 Files selected for processing (6)
src/frontend/src/modals/stepperModal/StepperModal.tsxsrc/frontend/src/modals/stepperModal/StepperProvider.tsxsrc/frontend/src/modals/stepperModal/hooks/__tests__/useStepperState.test.tsxsrc/frontend/src/modals/stepperModal/hooks/useStepperState.tssrc/frontend/src/modals/stepperModal/types.tssrc/frontend/src/pages/MainPage/pages/deploymentsPage/contexts/deployment-stepper-context.tsx
| function clampStep(step: number, minStep: number, totalSteps: number): number { | ||
| if (totalSteps === 0) return 0; | ||
| return Math.min(Math.max(step, minStep), totalSteps); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize 1-based numeric inputs before clamping.
minStep: 0 lets back() commit step 0; fractional or NaN values can produce a non-index currentStep. Normalize minStep to an integer in [1, totalSteps] and reject or normalize invalid numeric goTo/initial values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/src/modals/stepperModal/hooks/useStepperState.ts` around lines 8
- 10, Update clampStep to normalize minStep to an integer within [1, totalSteps]
before clamping, while retaining the zero-total-steps result of 0. Ensure back()
cannot commit step 0, and validate or normalize fractional, NaN, and other
invalid numeric inputs from goTo and initial state so currentStep is always a
valid 1-based index.
| const commitStep = useCallback( | ||
| (from: number, to: number) => { | ||
| if (to === from) return; | ||
| setRawStep(to); | ||
| onStepChange?.(from, to); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fire onStepChange only for committed transitions.
Batched calls such as next(); next() synchronously emit (1, 2) twice, although state commits only once. Notify after the committed state change and add a regression test for batched transitions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/src/modals/stepperModal/hooks/useStepperState.ts` around lines
42 - 46, Update commitStep in useStepperState so onStepChange is invoked only
after the corresponding state update is committed, preventing batched next()
calls from emitting duplicate transitions; preserve the no-op behavior when from
equals to, and add a regression test covering batched transitions.
| export function StepperProvider({ | ||
| children, | ||
| ...options | ||
| }: StepperProviderProps) { | ||
| const state = useStepperState(options); | ||
| return ( | ||
| <StepperStateContext.Provider value={state}> | ||
| {children} | ||
| </StepperStateContext.Provider> | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
rg -n -C 3 --glob '*.{ts,tsx}' 'zustand|createStore\(' src/frontend/srcRepository: langflow-ai/langflow
Length of output: 35983
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== candidate files =="
fd -a 'StepperProvider.tsx|Stepper.*\.ts|Stepper.*\.tsx|stepper.*Store' src/frontend/src | sed 's#^\./##'
echo
echo "== StepperProvider =="
file="$(fd 'StepperProvider.tsx' src/frontend/src | head -n 1 || true)"
if [ -n "${file:-}" ]; then
cat -n "$file"
fi
echo
echo "== stepper-related files =="
rg -n -C 4 --glob '*.ts' --glob '*.tsx' 'StepperProvider|useStepperState|StepperStateContext|useStepper' src/frontend/src || trueRepository: langflow-ai/langflow
Length of output: 50376
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
root = Path('src/frontend/src/modals/stepperModal')
candidates = list(root.glob('*'))
for p in candidates:
if p.is_file():
print(p.relative_to('src/frontend/src/modals/stepperModal'))
PYRepository: langflow-ai/langflow
Length of output: 217
Use Zustand for the stepper’s provider-scoped state.
StepperProvider manages navigation state inside useStepperState, so each provider instance owns its own mutable state outside the frontend’s Zustand-store convention. Move this to a provider-scoped Zustand store while keeping instances isolated via key/instance store selection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/src/modals/stepperModal/StepperProvider.tsx` around lines 13 -
23, Update StepperProvider and useStepperState so navigation state is held in a
provider-scoped Zustand store rather than local hook-managed mutable state.
Create or select the store per provider instance using the existing key/instance
mechanism, keep separate StepperProvider instances isolated, and expose the
selected store state through StepperStateContext without changing the provider’s
public behavior.
Source: Coding guidelines
| // Navigation runs on the shared stepper primitive. Conditional steps are | ||
| // the computed-list model: edit mode omits the Provider step entirely | ||
| // (3 steps: Type → Attach → Review), so no index shifting is needed. | ||
| const steps = useMemo<StepperStepConfig[]>( | ||
| () => | ||
| isEditMode | ||
| ? [{ id: "type" }, { id: "flows" }, { id: "review" }] | ||
| : [ | ||
| { id: "provider" }, | ||
| { id: "type" }, | ||
| { id: "flows" }, | ||
| { id: "review" }, | ||
| ], | ||
| [isEditMode], | ||
| ); | ||
|
|
||
| const stepper = useStepperState({ steps, initialStep: minStep, minStep }); | ||
| const { currentStep, totalSteps } = stepper; | ||
|
|
||
| const canGoNext = useMemo(() => { | ||
| const logical = getLogicalStep(currentStep); | ||
| if (logical === 1) { | ||
| return ( | ||
| selectedProvider !== null && | ||
| (selectedInstance !== null || hasValidCredentials) | ||
| ); | ||
| } | ||
| if (logical === 2) { | ||
| return isDeploymentNameValid && selectedLlm.trim() !== ""; | ||
| } | ||
| if (logical === 3) { | ||
| // In edit mode, user can proceed without new attachments (may just change desc/LLM). | ||
| return isEditMode || selectedVersionByFlow.size > 0; | ||
| } | ||
| if (logical === 4) { | ||
| return !hasToolNameErrors; | ||
| switch (stepper.currentStepId) { | ||
| case "provider": | ||
| return ( | ||
| selectedProvider !== null && | ||
| (selectedInstance !== null || hasValidCredentials) | ||
| ); | ||
| case "type": | ||
| return isDeploymentNameValid && selectedLlm.trim() !== ""; | ||
| case "flows": | ||
| // In edit mode, user can proceed without new attachments (may just change desc/LLM). | ||
| return isEditMode || selectedVersionByFlow.size > 0; | ||
| case "review": | ||
| return !hasToolNameErrors; | ||
| default: | ||
| return true; | ||
| } | ||
| return true; | ||
| }, [ | ||
| currentStep, | ||
| getLogicalStep, | ||
| stepper.currentStepId, | ||
| selectedProvider, | ||
| selectedInstance, | ||
| hasValidCredentials, | ||
| deploymentName, | ||
| isDeploymentNameValid, | ||
| selectedLlm, | ||
| selectedVersionByFlow, | ||
| isEditMode, | ||
| hasToolNameErrors, | ||
| ]); | ||
|
|
||
| // Ungated like the setState machine it replaces — the per-step gate is | ||
| // canGoNext, consumed by the footer's disabled state; goTo clamps to | ||
| // [minStep, totalSteps]. | ||
| const handleNext = useCallback(() => { | ||
| setCurrentStep((prev) => (prev < totalSteps ? prev + 1 : prev)); | ||
| }, [totalSteps]); | ||
| stepper.goTo(stepper.currentStep + 1); | ||
| }, [stepper]); | ||
|
|
||
| const handleBack = useCallback(() => { | ||
| setCurrentStep((prev) => (prev > minStep ? prev - 1 : prev)); | ||
| }, [minStep]); | ||
| const handleBack = stepper.back; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate relevant files =="
fd -a 'deployment-stepper-context\.tsx|useStepperState\.ts|stepper' . | sed 's#^\./##' | head -100
echo
echo "== Main file outline/imports/context =="
file="src/frontend/src/pages/MainPage/pages/deploymentsPage/contexts/deployment-stepper-context.tsx"
if [ -f "$file" ]; then
wc -l "$file"
ast-grep outline "$file" --view compact || true
sed -n '1,360p' "$file" | cat -n
fi
echo
echo "== Stepper hook candidates =="
for f in $(fd 'useStepperState\.ts|stepper.*\.(ts|tsx)$' src/frontend || true); do
echo "--- $f"
wc -l "$f"
ast-grep outline "$f" --view compact || true
sed -n '1,260p' "$f" | cat -n
doneRepository: langflow-ai/langflow
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deployment stepper main component outline =="
ast-grep outline "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployment-stepper.tsx" --view signatures || true
echo
echo "== deployment-stepper.tsx relevant keyboard/modal call sites =="
rg -n "handleNext|handleKeyDown|onPressEnter|Enter|keyDown|onKeyDown|canGoNext|onPrimaryAction|handlePrimary" "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployment-stepper.tsx" -C 4
sed -n '1,260p' "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployment-stepper.tsx" | cat -n
echo
echo "== all handleNext call sites in deployments page =="
rg -n "handleNext\(\)|handleNext\s*=" "src/frontend/src/pages/MainPage/pages/deploymentsPage" -C 3
echo
echo "== all KeyboardEvent call sites in stepper modal components =="
rg -n "KeyboardEvent|onKeyDown|onKeyPress|handleKeyDown|enter" "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/*.tsx" -C 3
echo
echo "== payload builder concern excerpt =="
sed -n '360,460p' "src/frontend/src/pages/MainPage/pages/deploymentsPage/contexts/deployment-stepper-context.tsx" | cat -nRepository: langflow-ai/langflow
Length of output: 640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deployment-stepper.tsx all relevant references =="
rg -n "handleNext|handleBack|onPrimary|onBack|useDeploymentStepper|handleKeyDown|onKeyDown|onKeyPress|keyboard|Enter|key" "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployment-stepper.tsx" -C 4 || true
echo
echo "== deployment-stepper.tsx first 220 lines =="
sed -n '1,220p' "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployment-stepper.tsx" | cat -n
echo
echo "== deployments page handleNext call sites with imports =="
rg -n "handleNext|useDeploymentStepper|canGoNext" "src/frontend/src/pages/MainPage/pages/deploymentsPage" -C 3
echo
echo "== all KeyboardEvent call sites in deployments page components =="
rg -n "KeyboardEvent|onKeyDown|onKeyPress|handleKeyDown|Enter|key" "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/*.tsx" -C 3Repository: langflow-ai/langflow
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DeploymentStepperModal lines 240-310 (handleNext call) =="
sed -n '240,310p' "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployment-stepper-modal.tsx" | cat -n
echo
echo "== deployment-stepper-footer props in DeploymentStepperModal =="
sed -n '380,410p' "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployment-stepper-modal.tsx" | cat -n
echo
echo "== deployment-steps footer props =="
sed -n '1,240p' "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployment-steps.tsx" | cat -n
rg -n "handleNext|handleBack|onPrimary|onBack|canGoNext" "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployment-steps.tsx" -C 2 || true
echo
echo "== all programmatic handleNext usage =="
rg -n "handleNext\s*\(" "src/frontend/src/pages/MainPage/pages/deploymentsPage" -C 3Repository: langflow-ai/langflow
Length of output: 4567
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deployment-stepper-modal full keyboard/modal dialog setup =="
rg -n "onOpenChange|onClose|onCloseAutoFocus|onEscapeKeyDown|onSelect|onKeyDown|onKeyPress|onEnter|Enter|handleOpen|setOpen|Modal|StepperContent" "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployment-stepper-modal.tsx" -C 5 || true
echo
echo "== deployment-stepper-modal all handleNext references =="
rg -n "handleNext\(\)|handleStepNext|keyboard|on.*key" "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployment-stepper-modal.tsx" -C 3 || true
echo
echo "== focused deployment-stepper-modal relevant section =="
sed -n '1,120p' "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployment-stepper-modal.tsx" | cat -n
sed -n '330,430p' "src/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployment-stepper-modal.tsx" | cat -n
echo
echo "== tests covering handleNext bypass behavior =="
rg -n "handleNext\([^)]*not|!canGoNext|blocks when .*,|blocks when|handleNext" "src/frontend/src/pages/MainPage/pages/deploymentsPage/__tests__" -C 3 || trueRepository: langflow-ai/langflow
Length of output: 35152
Gate handleNext before using the stepper primitive.
handleNext currently calls stepper.goTo(stepper.currentStep + 1) without canGoNext, while canGoNext is only used to disable the footer button. Since useStepperState.goTo() ignores per-step canNext predicates, exposing handleNext on the public context lets invalid state advance steps and can feed incomplete data into buildDeploymentPayload/buildDeploymentUpdatePayload. Guard it with the local gate, or rely on the shared stepper next()/step canNext configuration instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/frontend/src/pages/MainPage/pages/deploymentsPage/contexts/deployment-stepper-context.tsx`
around lines 251 - 306, Guard handleNext with canGoNext before advancing the
stepper, so invalid state cannot move to the next step through the public
context. Preserve the existing stepper.goTo increment for valid transitions and
keep the current canGoNext predicates unchanged.
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
Cristhianzl
left a comment
Cristhianzl
left a comment
There was a problem hiding this comment.
⚠️ Important (preferably this PR)
I1 — The framework's headline capability is not used by its "first production consumer"
File: src/frontend/src/pages/MainPage/pages/deploymentsPage/contexts/deployment-stepper-context.tsx:254-306
Issue: The capability table in the description maps "Per-step validation gate" to canNext per step config. The adopted code declares its steps without any canNext ([{ id: "type" }, { id: "flows" }, { id: "review" }]), computes canGoNext itself in a separate useMemo, and routes handleNext through the ungated goTo rather than next() — deliberately, and honestly documented in the comment at line 299-301. So stepper.canGoNext is unconditionally true and stepper.next() is never called.
The same holds for the rest of the API: isBusy is never passed (the deployment flow's isDeploying / isSubmitting / isCreatingAccount stay outside the primitive), onStepChange is never passed, reset() is never called, progressPercent is never read, and StepperProvider / useStepper have no production call site — the deployment context keeps its own React context.
Why it matters: Roughly half of a 119-line hook plus a 31-line provider plus 63 lines of types are speculative surface (YAGNI), and the API-validated-against-real-use argument is what justifies landing them now. The risk is concrete: the deploy-choice dialog is the consumer these features were designed for, and its needs are being inferred rather than observed. If canNext turns out to want async validation, or isBusy needs to gate reset too, the API changes with a released consumer already on it.
Suggested fix: Pick one. (a) Wire the real gate through the primitive in this PR — move the four canGoNext clauses into canNext on each step config and have handleNext call stepper.next(). That preserves observable behavior (the footer already disables on canGoNext) and makes the flagship feature load-bearing. Or (b) ship only steps/goTo/back/currentStepId now and land canNext, isBusy, onStepChange, reset, progressPercent and StepperProvider with the deploy-choice PR that actually needs them. Option (a) is the smaller change and the stronger validation.
Code reference
const stepper = useStepperState({ steps, initialStep: minStep, minStep });
// …
const handleNext = useCallback(() => {
stepper.goTo(stepper.currentStep + 1); // never stepper.next()
}, [stepper]);I2 — handleNext now churns identity on every step change, through the context value
File: src/frontend/src/pages/MainPage/pages/deploymentsPage/contexts/deployment-stepper-context.tsx:302-304
Issue: useCallback(…, [stepper]) where stepper is the memoized state object whose deps include currentStep, canGoNext, canGoBack and progressPercent. Every transition — and every re-render that flips canGoNext — produces a new stepper, therefore a new handleNext, which goes into the context value at lines 422 and 468.
Before this PR, handleNext depended on [totalSteps] and was stable for the entire wizard.
Why it matters: Every component reading useDeploymentStepper() re-renders on each transition even if it only consumes handleNext — that includes the footer and the step panels. Not a correctness bug, but it's a regression introduced by the refactor in a context that already fans out widely, and it quietly undercuts the memoization the rest of the file maintains.
Suggested fix: Depend on the stable pieces rather than the container: useCallback(() => stepper.goTo(stepper.currentStep + 1), [stepper.goTo, stepper.currentStep]) still churns on step change. The clean fix pairs with I1(a) — const handleNext = stepper.next is stable across renders where canGoNext and currentStep hold, exactly like handleBack = stepper.back on line 306.
💡 Recommended (can ship as a follow-up)
R1 — A stale rawStep survives a shrinking step list and resurfaces if the list regrows
File: src/frontend/src/modals/stepperModal/hooks/useStepperState.ts:36-49 · locked in by hooks/__tests__/useStepperState.test.tsx:183-198
Issue: currentStep is clampStep(rawStep, …) but commitStep early-returns when to === from computed from the clamped value. So with rawStep = 4 and a list that shrank to 2 entries, goTo(2) is a no-op — rawStep stays 4 — and the moment the list regrows to 4 the stepper jumps straight back to step 4. The test at line 183 asserts this as intended ("restores when it grows back").
Why it matters: It's a defensible design for an auto-skip that toggles back, and it's unreachable in the deployment stepper (isEditMode is fixed for the life of the provider). But it's a genuine footgun for the deploy-choice dialog in the next PR, where the phase list is conditional on user selections — a user who backs out of a branch can find themselves teleported forward when the branch re-enables. The behavior is documented in the hook's docblock but not on StepperState.currentStep, where a consumer would look.
Suggested fix: Document the "underlying position is preserved" contract on the currentStep field in types.ts alongside the docblock, and expose rawStep (or a clearPendingStep()) so a consumer that wants shrink-to-be-permanent can opt out. At minimum, make goTo write rawStep even when the clamped value is unchanged.
R2 — canNext is invoked during render with no purity contract
File: src/frontend/src/modals/stepperModal/hooks/useStepperState.ts:13-16, 51-52
resolveCanNext(currentStepConfig) runs on every render as part of the canGoNext expression, calling the consumer's function form. The test at useStepperState.test.tsx:151-164 relies on exactly that (re-evaluated per render). Nothing tells a consumer the function must be pure and side-effect-free — a canNext that touches a store, logs, or triggers validation will fire on every render including ones caused by unrelated state.
Suggested fix: Add the constraint to the StepperStepConfig.canNext JSDoc: "Evaluated on every render — must be pure."
R3 — goTo(0) is silently treated as "step id not found"
File: src/frontend/src/modals/stepperModal/hooks/useStepperState.ts:66-73
findIndex(...) + 1 yields 0 for an unknown id, and the guard if (target < 1) return; handles it. But that same guard swallows a numeric goTo(0) — a plausible off-by-one from a consumer used to 0-based indexing — which should clamp to minStep under the documented "clamped to [minStep, total]" contract. Two different intents share one sentinel.
Suggested fix: Resolve the id branch to -1 explicitly and guard if (target === -1) return;, letting numeric inputs fall through to clampStep. One test each for goTo(0) and goTo(-1).
R4 — deployment-stepper-context.tsx crosses the 500-LOC limit
File: src/frontend/src/pages/MainPage/pages/deploymentsPage/contexts/deployment-stepper-context.tsx (513 LOC)
The file was already near the ceiling and this PR pushes it to 513, over the 500-LOC baseline in .claude/CLAUDE.md (500–700 acceptable only when single-responsibility clearly holds — this file currently owns navigation, provider/credentials state, flow-attachment state, name validation and payload building). Not introduced by this PR, but it grew here. Now that navigation is a one-liner on the shared primitive, splitting the domain state out is a much smaller job than it was. Worth a follow-up ticket alongside the WP4 extractions.
🔹 Nice-to-have
N1 — The module has no index.ts, so consumers import from a file that looks internal
File: src/frontend/src/pages/MainPage/pages/deploymentsPage/contexts/deployment-stepper-context.tsx:14-17
import { type StepperStepConfig, useStepperState } from "@/modals/stepperModal/StepperModal" reaches into the component file for a hook and a type. StepperModal.tsx re-exports the public API deliberately (lines 125-140), so it works — but the folder has no index.ts, and importing a headless hook from a file named after a modal reads as an accident. Adding modals/stepperModal/index.ts that re-exports the same set would make the public surface explicit and let consumers write @/modals/stepperModal.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-1.12.0 #14284 +/- ##
==================================================
+ Coverage 61.52% 62.12% +0.59%
==================================================
Files 2349 2347 -2
Lines 238463 238193 -270
Branches 33541 35523 +1982
==================================================
+ Hits 146719 147969 +1250
+ Misses 89929 88409 -1520
Partials 1815 1815
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ep gate Wires the deployment stepper's advance gates through the shared primitive (review I1/I2): each advancing step carries its own canNext, so canGoNext and handleNext come from useStepperState instead of a parallel canGoNext useMemo and an ungated goTo. handleNext is now stepper.next — stable except when canGoNext/currentStep change, replacing the whole-stepper dependency that churned on every transition. The Review step keeps no canNext: it is the last step and its primary action is Deploy, so its button gate stays hasToolNameErrors, applied via isLastStep. The primitive's canGoNext is false on the last step by design, so folding the review clause into canNext would have disabled Deploy. One navigation test now seeds each step to reach step 4, since handleNext is gated; the clamp assertion itself is unchanged. Refs: LE-1736
The function form is evaluated on every render as part of canGoNext, so a canNext that touches a store, logs or triggers validation would fire on unrelated re-renders. Document the purity constraint (review R2). Refs: LE-1736
Adds modals/stepperModal/index.ts re-exporting the public surface (the hooks, provider and types StepperModal.tsx already re-exports, plus the default) so the headless deployment consumer imports from @/modals/stepperModal instead of reaching into the modal component file. Review N1. Refs: LE-1736
github-actions
Bot
added
refactor
github-actions
Bot
added
refactor
2 participants
Why
The frontend carries four independent step/phase machines — the
modals/stepperModalvisual shell (knowledge-base upload wizard), the deploymentsPage stepper (deployment-stepper-modal.tsx+deployment-stepper-context.tsx+ footer), theWizardStepstate insideuseKnowledgeBaseForm, and theuse-deploy-choice-dialog-statephase machine — roughly 250-400 LOC of duplicated modal shell, next/back/finish footer and step-index/progress logic. Todaymodals/stepperModalonly ships the visuals: its context merely echoescurrentStep/totalSteps/title/description, and every consumer hand-rolls its own transition state. This PR promotes the module into the canonical stepper framework and rebuilds the deployment stepper's navigation on it — the framework lands together with its first production consumer, so the API is reviewed against real use, not in the abstract. The deploy-choice phase machine follows in the next PR of the sequence.What
Additive framework layer inside
modals/stepperModal:hooks/useStepperState.ts— NEW: headless transition logic. 1-basedcurrentStepwith clamping,next()/back()/goTo(idOrNumber), per-step validation gate (canNextas boolean or lazily evaluated function),minStepback floor independent frominitialStep, consumer-ownedisBusygating every transition,onStepChange(from, to)fired only on committed transitions,reset(), andprogressPercentusing the indicators'(current-1)/(total-1)formula (single-step guard included). Conditional steps are expressed as a computedstepslist; if the list shrinks below the current position the exposed step clamps without losing the underlying one.StepperProvider.tsx— NEW: context wrapper over the hook plus auseStepper()accessor; shell-less flows (the deploy-choice dialog, next PR) can calluseStepperStatedirectly.types.ts—StepperStepConfig,UseStepperStateOptions,StepperState,StepperProviderPropsadded.StepperModal.tsx— re-exports the new API from the module entry (public API unchanged otherwise).First consumer — the deployment stepper runs its navigation on the shared primitive:
deploymentsPage/contexts/deployment-stepper-context.tsx— the hand-rolleduseStatestep machine, next/back clamps, per-modetotalStepsand the edit-modegetLogicalStepindex shifting are replaced byuseStepperStatewith a computed step list (edit mode simply omits the Provider step:[type, flows, review]vs[provider, type, flows, review]). The publicuseDeploymentStepper()API is unchanged —currentStep,totalSteps,minStep,canGoNext,handleNext,handleBackkeep their exact semantics:canGoNextstays the per-step domain gate (now keyed by step id instead of shifted numbers) andhandleNextstays ungated with the same clamping, matching the machine it replaces. Step panels, modal, footer and indicator are untouched.The API shape was designed from the real consumers before any code:
stepsconfig list,currentStep,currentStepId,totalStepsstepslist + shrink clampingcanGoNextmemo per logical step in the deployment contextcanNextper step config; gatednext(); exposedcanGoNextminStepinitialStep+ independentminStep; exposedcanGoBackprovider/deployments/review(next PR)ids +goTo,initialStepwithminStep: 1isDeploying/isSubmitting/isCreatingAccountdisable navigationisBusyoption gates all transitions (busy stays consumer-owned)reset()Untouched on purpose (ticket rules):
StepperModalshell,StepperModalFooter,KnowledgeBaseUploadModalanduseKnowledgeBaseForm(its wizard state migrates in the Tier C decomposition — this PR only guarantees the primitive can host it).How to validate
Two surfaces, one walkthrough each. The KB upload wizard has zero rewired code, so its check is the walkthrough staying IDENTICAL; the deployment stepper runs on the new primitive with unchanged semantics, so its walkthrough must also be indistinguishable from the base. The evidence screenshots are numbered 1:1 with these steps (compare step Sn with image Sn).
Knowledge page → Add Knowledge (needs at least one configured embedding provider):
Deployments tab (Beta; behind the wxo_deployments feature flag) → Create Deployment, with one watsonx environment configured and at least one flow with a version:
Edit mode (3 steps, Provider omitted) is covered by the 31-file deploymentsPage unit suite —
totalSteps/step-shifting assertions run against the rebuilt context unchanged. Plus: devtools console shows zero new errors vs baseline throughout.Evidence — KB upload wizard walkthrough (S1-S10, before | after)
Both worlds ran the same capture script (network-mocked KB list, two embedding models, deterministic chunk-preview payload), so the pairs are byte-comparable; identical pairs are the point — this consumer is untouched.
Evidence — deployment stepper walkthrough (S11-S20, before | after)
Both worlds ran the same capture script (real backend for flows/versions; deployments endpoints network-mocked — the OSS backend has no watsonx account), so the pairs are byte-comparable; the rebuilt navigation must be indistinguishable from the base.
Tests
stepperModal/hooks/__tests__/useStepperState.test.tsx— 26 tests covering the transition logic: initial clamping, next/back with boolean and lazily re-evaluated function gates, minStep floor vs free back after an auto-skip opening, goTo by id and clamped number, conditional list shrink/regrow, busy gating with reset still available, onStepChange only on committed transitions, progress formula with the single-step division-by-zero guard, provider/useStepper wiring.make test_frontendfrom the repo root: 5,528 tests, 100% green.npx playwright test --list: clean collection.npx biome checkclean on the touched files;tsc --noEmitintroduces zero new errors on touched files (repo-wide baseline diagnostics reproduce identically on the base — verified by running tsc on both trees and diffing the area output).git diff --name-only origin/release-1.12.0= exactly the 6 expected files (5 framework/test + 1 deployment context).Note
Net diff is +565 (+230 production): the framework is additive and the deployment context sheds only its navigation internals here; WP2's net-negative lands as the remaining machines migrate (deploy-choice next). The S3 evidence shows only the name error because the model input auto-selects the first embedding option whenever the field is empty — pre-existing behavior this PR does not touch. The deployment capture mocks the deployments/* endpoints (no watsonx account in OSS backends) while flows/versions/snapshots are real.
Refs: LE-1736
Summary by CodeRabbit
New Features
Improvements