feat(form): multi-step form wizard - #726
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds multi-step wizard support: a ChangesMulti-Step Wizard Implementation
Sequence Diagram(s)sequenceDiagram
participant FormWidgetRenderer
participant useFormWizard
participant validateStepFields
participant FormStepIndicator
FormWidgetRenderer->>useFormWizard: initialize(fields, chartOptions)
FormWidgetRenderer->>FormStepIndicator: render(stepLabels, currentStep)
FormWidgetRenderer->>useFormWizard: goNext()
useFormWizard->>validateStepFields: validateStepFields(currentStepFields, localValues)
alt valid
useFormWizard->>FormWidgetRenderer: advance step (no errors)
else invalid
useFormWizard-->>FormWidgetRenderer: return errors
end
FormWidgetRenderer->>useFormWizard: reset() on successful submit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/src/hooks/use-form-wizard.ts (1)
50-58: ⚡ Quick winOptimize
stepLabelsmemoization by movingconfiguredLabelsextraction insideuseMemo.Currently,
configuredLabelscreates a new empty array[]on every render whenchartOptions.stepLabelsis undefined. This new reference causes thestepLabelsuseMemoto recalculate unnecessarily even when nothing has changed, defeating the optimization.♻️ Proposed fix
- const configuredLabels = - (chartOptions.stepLabels as string[] | undefined) ?? []; const stepLabels = useMemo(() => { + const configuredLabels = + (chartOptions.stepLabels as string[] | undefined) ?? []; const labels = stepGroups.map( (_, i) => configuredLabels[i] || `Step ${i + 1}`, ); if (enableSummary) labels.push("Review"); return labels; - }, [stepGroups, configuredLabels, enableSummary]); + }, [stepGroups, chartOptions.stepLabels, enableSummary]);🤖 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 `@app/src/hooks/use-form-wizard.ts` around lines 50 - 58, The current extraction of configuredLabels creates a new [] on every render which invalidates the stepLabels useMemo; move the configuredLabels computation inside the useMemo that defines stepLabels so it only creates the fallback array when memo runs, and update the dependency list to reference chartOptions.stepLabels (or chartOptions) and enableSummary and stepGroups instead of the external configuredLabels; locate the code around the useMemo for stepLabels and the chartOptions.stepLabels usage to apply this change.
🤖 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 `@app/src/components/form-widget-renderer.tsx`:
- Around line 314-324: The generic Array.isArray(value) early-return prevents
the number-range formatting in form-widget-renderer.tsx from ever running;
change the logic so number-range arrays are handled first (or special-case them
inside the array branch): check field.parameterType === "number-range" when
value is an array and return `${value[0]} – ${value[1]}` (use the existing
number-range formatting) before falling back to value.join(", ") || "—"; update
the branches around the Array.isArray(value) and field.parameterType checks to
ensure the number-range path is reachable.
In `@app/src/components/widget-editor/form-fields-editor.tsx`:
- Around line 187-193: The step input handler currently uses
parseInt(e.target.value, 10) without NaN checks, which allows NaN to be stored
and later dropped by groupFieldsByStep; update the onChange in
form-fields-editor.tsx so that after parseInt you validate Number.isFinite or
!Number.isNaN and only pass a numeric step to onUpdate (otherwise pass
undefined/null), referencing the onChange callback that calls onUpdate(field.id,
{ step: ... }) and the parseInt usage; also ensure groupFieldsByStep in
form-field-def.ts treats non-numeric f.step the same as undefined (coerce NaN to
undefined or use Number.isFinite before comparing) so fields with invalid steps
are not lost.
In `@app/src/hooks/use-form-wizard.ts`:
- Around line 81-86: goToStep currently only allows stepping backward but lacks
bounds checks; update the goToStep callback to validate 0 <= step < max (e.g.,
use the wizard's totalSteps or steps.length) before calling setCurrentStep and
preserve the existing backward-only behavior (allow change only if step <
currentStep and within bounds). Reference the goToStep function and the state
setters currentStep and setCurrentStep; also update the useCallback dependencies
to include the totalSteps/steps length you use for the upper bound.
In `@component/src/components/composed/form-step-indicator.tsx`:
- Around line 37-42: The connector before the current step is using isCompleted
(computed as idx < currentStep) so the link for idx === currentStep renders as
upcoming; inside the FormStepIndicator component update the connector logic to
treat the connector as completed when idx <= currentStep (e.g., compute a
separate connectorCompleted = idx <= currentStep or adjust the condition used in
className for the connector) so the connector immediately before the active step
is rendered as completed.
---
Nitpick comments:
In `@app/src/hooks/use-form-wizard.ts`:
- Around line 50-58: The current extraction of configuredLabels creates a new []
on every render which invalidates the stepLabels useMemo; move the
configuredLabels computation inside the useMemo that defines stepLabels so it
only creates the fallback array when memo runs, and update the dependency list
to reference chartOptions.stepLabels (or chartOptions) and enableSummary and
stepGroups instead of the external configuredLabels; locate the code around the
useMemo for stepLabels and the chartOptions.stepLabels usage to apply this
change.
🪄 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
Run ID: 559d81df-f592-4a2f-bfb3-e9056a9b535a
📒 Files selected for processing (10)
app/src/components/form-widget-renderer.tsxapp/src/components/widget-editor-modal.tsxapp/src/components/widget-editor/form-fields-editor.tsxapp/src/hooks/use-form-wizard.tsapp/src/lib/__tests__/widget/form-field-def.test.tsapp/src/lib/__tests__/widget/form-field-validation.test.tsapp/src/lib/widget/form-field-def.tsapp/src/lib/widget/form-field-validation.tscomponent/src/components/composed/form-step-indicator.tsxcomponent/src/components/composed/index.ts
| {idx > 0 && ( | ||
| <div | ||
| className={cn( | ||
| "h-px flex-1 min-w-2", | ||
| isCompleted ? "bg-primary" : "bg-border", | ||
| )} |
There was a problem hiding this comment.
Connector state is off by one for the current step.
At Line 41, the connector before the current step renders as upcoming because it reuses isCompleted (idx < currentStep). For idx === currentStep, that link should already be completed.
Proposed fix
{stepLabels.map((label, idx) => {
const isCompleted = idx < currentStep;
const isCurrent = idx === currentStep;
+ const isConnectorCompleted = idx <= currentStep;
return (
<React.Fragment key={idx}>
{idx > 0 && (
<div
className={cn(
"h-px flex-1 min-w-2",
- isCompleted ? "bg-primary" : "bg-border",
+ isConnectorCompleted ? "bg-primary" : "bg-border",
)}
/>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {idx > 0 && ( | |
| <div | |
| className={cn( | |
| "h-px flex-1 min-w-2", | |
| isCompleted ? "bg-primary" : "bg-border", | |
| )} | |
| {stepLabels.map((label, idx) => { | |
| const isCompleted = idx < currentStep; | |
| const isCurrent = idx === currentStep; | |
| const isConnectorCompleted = idx <= currentStep; | |
| return ( | |
| <React.Fragment key={idx}> | |
| {idx > 0 && ( | |
| <div | |
| className={cn( | |
| "h-px flex-1 min-w-2", | |
| isConnectorCompleted ? "bg-primary" : "bg-border", | |
| )} | |
| /> | |
| )} |
🤖 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 `@component/src/components/composed/form-step-indicator.tsx` around lines 37 -
42, The connector before the current step is using isCompleted (computed as idx
< currentStep) so the link for idx === currentStep renders as upcoming; inside
the FormStepIndicator component update the connector logic to treat the
connector as completed when idx <= currentStep (e.g., compute a separate
connectorCompleted = idx <= currentStep or adjust the condition used in
className for the connector) so the connector immediately before the active step
is rendered as completed.
Add optional multi-step wizard mode to the form widget. Fields can be assigned to numbered steps, enabling guided data entry workflows. Data model: - FormFieldDef gains optional `step?: number` field (no DB migration) - chartOptions.stepLabels: string[] for custom step names - chartOptions.enableSummary: boolean (default true when steps exist) New components: - FormStepIndicator (component/) — progress bar with completed/current states - useFormWizard hook — manages step navigation, per-step validation, reset Renderer changes: - Wizard mode: shows step indicator, renders only current step's fields - Next button validates current step before advancing - Back button navigates freely (no validation) - Summary step shows all values for review before submit - Non-wizard forms render unchanged (backward compatible) Editor changes: - Step number input per field in form-fields-editor - Step label inputs in advanced tab (shown when steps are assigned) Closes #165 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6289f09 to
95ac404
Compare
Skip per-keystroke delays in login page tests that timed out in CI. Add unit tests for useFormWizard hook, FormStepIndicator component, and FormWidgetRenderer wizard mode to satisfy SonarCloud 80% new code coverage gate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/src/components/__tests__/form-widget-renderer-wizard.test.tsx (2)
193-195: 💤 Low valueRedundant assertion:
toBeDefined()aftergetByTestId.
screen.getByTestId()throws if the element is not found, so the.toBeDefined()assertion on lines 193-195 (and similar patterns throughout the file) is redundant. Consider removing.toBeDefined()or using.toBeInTheDocument()from@testing-library/jest-dommatchers for clarity.🤖 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 `@app/src/components/__tests__/form-widget-renderer-wizard.test.tsx` around lines 193 - 195, Remove the redundant .toBeDefined() assertions that follow calls like screen.getByTestId("form-step-indicator") and screen.getByText("Step 1"/"Step 2"); either drop the .toBeDefined() completely (since getBy* throws if missing) or replace them with the clearer matcher .toBeInTheDocument() from `@testing-library/jest-dom` for explicit presence checks in the tests referencing these selectors.
1-1: 💤 Low valueConsider adding explicit jsdom environment directive.
The test file
app/src/hooks/__tests__/use-form-wizard.test.tsincludes//@vitest-environmentjsdomon line 1, but this file does not. For consistency and explicit environment declaration, consider adding the directive here as well.📝 Proposed addition
+// `@vitest-environment` jsdom import { describe, it, expect, vi, beforeEach } from "vitest";🤖 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 `@app/src/components/__tests__/form-widget-renderer-wizard.test.tsx` at line 1, Add an explicit Vitest jsdom environment directive at the top of the test file by inserting the comment "// `@vitest-environment` jsdom" as the very first line (above the existing import line that starts with `import { describe, it, expect, vi, beforeEach } from "vitest";`) so the test explicitly runs under jsdom like the other hook test.component/src/components/composed/__tests__/form-step-indicator.test.tsx (1)
156-189: ⚖️ Poor tradeoffConsider using data attributes for connector queries.
The tests query connectors using CSS class selectors (
.h-px.flex-1), which couples tests to Tailwind implementation details. If styling classes change, tests will break even though behavior is correct.Consider adding
data-testidattributes to connector elements for more resilient querying, or document that these tests intentionally verify styling classes.🤖 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 `@component/src/components/composed/__tests__/form-step-indicator.test.tsx` around lines 156 - 189, The tests are brittle because they select connectors by Tailwind classes; update the FormStepIndicator implementation to add a stable data attribute (e.g. data-testid="connector") to each connector element, then change the tests in form-step-indicator.test.tsx to query connectors by that attribute (e.g. container.querySelectorAll('[data-testid="connector"]') or getAllByTestId) and assert classes or state as before; ensure the attribute is added where connectors are rendered in the FormStepIndicator component so all three tests (render count, completed vs border color checks, upcoming checks) use the new selector.
🤖 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 `@app/src/components/__tests__/form-widget-renderer-wizard.test.tsx`:
- Around line 450-492: The test claims to check formatSummaryValue but only
asserts the field label; either (A) change the test to actually assert the
formatted array by having mockUseFormWizard return localValues (or whatever
prop/state FormWidgetRenderer reads) with parameterName "tags" mapped to an
array (e.g., ["a","b"]) and then render FormWidgetRenderer and expect the
summary cell to contain the comma-separated string "a, b" (verifying
formatSummaryValue output), or (B) rename the test and its description from
"formats array values as comma-separated" to "renders multi-select field label
on summary step" to reflect that it only verifies the label rendering; update
the test title and keep the existing assertion (screen.getByText("Tags")).
Ensure you modify the mock returned by mockUseFormWizard and target the
parameterName "tags" or adjust the expectation accordingly if choosing option A.
---
Nitpick comments:
In `@app/src/components/__tests__/form-widget-renderer-wizard.test.tsx`:
- Around line 193-195: Remove the redundant .toBeDefined() assertions that
follow calls like screen.getByTestId("form-step-indicator") and
screen.getByText("Step 1"/"Step 2"); either drop the .toBeDefined() completely
(since getBy* throws if missing) or replace them with the clearer matcher
.toBeInTheDocument() from `@testing-library/jest-dom` for explicit presence checks
in the tests referencing these selectors.
- Line 1: Add an explicit Vitest jsdom environment directive at the top of the
test file by inserting the comment "// `@vitest-environment` jsdom" as the very
first line (above the existing import line that starts with `import { describe,
it, expect, vi, beforeEach } from "vitest";`) so the test explicitly runs under
jsdom like the other hook test.
In `@component/src/components/composed/__tests__/form-step-indicator.test.tsx`:
- Around line 156-189: The tests are brittle because they select connectors by
Tailwind classes; update the FormStepIndicator implementation to add a stable
data attribute (e.g. data-testid="connector") to each connector element, then
change the tests in form-step-indicator.test.tsx to query connectors by that
attribute (e.g. container.querySelectorAll('[data-testid="connector"]') or
getAllByTestId) and assert classes or state as before; ensure the attribute is
added where connectors are rendered in the FormStepIndicator component so all
three tests (render count, completed vs border color checks, upcoming checks)
use the new selector.
🪄 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
Run ID: 29e96416-30f7-4438-b713-7efd739b20b7
📒 Files selected for processing (4)
app/src/app/(auth)/login/__tests__/page.test.tsxapp/src/components/__tests__/form-widget-renderer-wizard.test.tsxapp/src/hooks/__tests__/use-form-wizard.test.tscomponent/src/components/composed/__tests__/form-step-indicator.test.tsx
- Fix unreachable number-range branch in formatSummaryValue by moving the check before the generic array early-return - Add NaN guard to step number input in form-fields-editor - Add bounds validation (step >= 0) to goToStep in useFormWizard - Remove unused import in form-step-indicator test - Rename misleading test description Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@app/src/hooks/use-form-wizard.ts`:
- Around line 66-75: The goNext callback can increment currentStep past the last
step causing an invalid wizard state; modify goNext (the function using
setCurrentStep) to clamp advancement by computing the max index from
stepGroups.length - 1 and calling setCurrentStep(s => Math.min(s + 1, maxIndex))
(or only increment when s < maxIndex), ensuring currentStep never exceeds the
terminal step; keep the existing validation via validateStepFields and return
semantics intact.
🪄 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
Run ID: c2898755-2f25-4ce8-84c8-d2d572293d2d
📒 Files selected for processing (6)
app/src/components/__tests__/form-widget-renderer-wizard.test.tsxapp/src/components/form-widget-renderer.tsxapp/src/components/widget-editor/form-fields-editor.tsxapp/src/hooks/__tests__/use-form-wizard.test.tsapp/src/hooks/use-form-wizard.tscomponent/src/components/composed/__tests__/form-step-indicator.test.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/components/widget-editor/form-fields-editor.tsx
- app/src/components/tests/form-widget-renderer-wizard.test.tsx
- app/src/components/form-widget-renderer.tsx
- component/src/components/composed/tests/form-step-indicator.test.tsx
- app/src/hooks/tests/use-form-wizard.test.ts
| const goNext = useCallback( | ||
| (localValues: Record<string, unknown>) => { | ||
| const stepFields = stepGroups[currentStep] ?? []; | ||
| const errors = validateStepFields(stepFields, localValues); | ||
| if (Object.keys(errors).length > 0) return errors; | ||
| setCurrentStep((s) => s + 1); | ||
| return null; | ||
| }, | ||
| [currentStep, stepGroups], | ||
| ); |
There was a problem hiding this comment.
Clamp goNext to the terminal step to avoid invalid wizard state.
At Line 71, setCurrentStep((s) => s + 1) can advance past the last valid step if goNext is triggered while already at the terminal step. Add a max-step guard and clamp the increment.
Suggested fix
const goNext = useCallback(
(localValues: Record<string, unknown>) => {
+ const maxStep = enableSummary ? totalSteps : totalSteps - 1;
+ if (currentStep >= maxStep) return null;
+
const stepFields = stepGroups[currentStep] ?? [];
const errors = validateStepFields(stepFields, localValues);
if (Object.keys(errors).length > 0) return errors;
- setCurrentStep((s) => s + 1);
+ setCurrentStep((s) => Math.min(s + 1, maxStep));
return null;
},
- [currentStep, stepGroups],
+ [currentStep, stepGroups, enableSummary, totalSteps],
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const goNext = useCallback( | |
| (localValues: Record<string, unknown>) => { | |
| const stepFields = stepGroups[currentStep] ?? []; | |
| const errors = validateStepFields(stepFields, localValues); | |
| if (Object.keys(errors).length > 0) return errors; | |
| setCurrentStep((s) => s + 1); | |
| return null; | |
| }, | |
| [currentStep, stepGroups], | |
| ); | |
| const goNext = useCallback( | |
| (localValues: Record<string, unknown>) => { | |
| const maxStep = enableSummary ? totalSteps : totalSteps - 1; | |
| if (currentStep >= maxStep) return null; | |
| const stepFields = stepGroups[currentStep] ?? []; | |
| const errors = validateStepFields(stepFields, localValues); | |
| if (Object.keys(errors).length > 0) return errors; | |
| setCurrentStep((s) => Math.min(s + 1, maxStep)); | |
| return null; | |
| }, | |
| [currentStep, stepGroups, enableSummary, totalSteps], | |
| ); |
🤖 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 `@app/src/hooks/use-form-wizard.ts` around lines 66 - 75, The goNext callback
can increment currentStep past the last step causing an invalid wizard state;
modify goNext (the function using setCurrentStep) to clamp advancement by
computing the max index from stepGroups.length - 1 and calling setCurrentStep(s
=> Math.min(s + 1, maxIndex)) (or only increment when s < maxIndex), ensuring
currentStep never exceeds the terminal step; keep the existing validation via
validateStepFields and return semantics intact.
Skip per-keystroke delays in signup page tests that timed out in CI. Fix TS spread error in wizard test mock. Export and add direct unit tests for formatSummaryValue covering all branches (number-range, multi-select, date-range, scalars, empty values). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Revert the export and drop the 15-test file that only existed to compensate for SonarCloud recounting lines after the source edit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove 4 TypeScript type-checking tests from advanced-connection-options that only verified compilation, not runtime behavior. Consolidate 5 repetitive date-range-picker preset tests into a single test.each, adding the missing 'Last 30 days' case in the process. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- connection: exclude dist/ from Jest testPathIgnorePatterns (fixes 2 spurious suite failures from compiled output) - app: rename chart-error-boundary.test.tsx → chart-renderer.test.tsx to match the component it actually tests - e2e/transforms: remove 1s sleep after connection select (Run button enabled check already gates); replace 1s post-save sleep with Save button re-enabled assertion - e2e/form-widget: remove duplicate 400ms debounce wait - e2e/widget-lab: replace 1s post-save sleep with Save button re-enabled assertion Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…zard # Conflicts: # app/src/components/form-widget-renderer.tsx # app/src/components/widget-editor-modal.tsx
|



Summary
Add optional multi-step wizard mode to the form widget. Admins assign step numbers to fields, enabling guided data entry with step-by-step navigation, per-step validation, and a summary review before submit.
Single-page mode (default — unchanged)
Forms without step assignments render exactly as before.
Wizard mode (when any field has a step number)
What's new
Key decisions
stepis optional, forms without it are unchangedTest plan
Closes #165
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores