Add submit (next page) simulation on form preview

[danielnaab]

Fixes #26

[changeset-bot]

⚠️ No Changeset found

Latest commit: ac15b38

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[claude]

Code Review

Summary

This PR adds submit/next page simulation to the form preview functionality by wrapping the preview content in a form element and implementing a handleSubmit function that navigates to the next page. This fixes the issue where the Next button wasn't functional in preview mode.

Positive Aspects

• Fixes the core issue: The preview now properly handles form submission and page navigation
• Minimal changes: The fix is focused and doesn't introduce unnecessary complexity
• Consistent structure: Both preview and non-preview modes now use proper form elements
• Code cleanup: Removed unnecessary React fragment wrapper in FormContents

Issues and Concerns

1. Loss of Form Data (High Priority)

The handleSubmit function in FormPreview.tsx:32-41 doesn't use the submitted form data. It only increments the page number without storing the user's input, updating the session state, or validating the form data before navigation.

Impact: Users will lose all their input when navigating between pages in preview mode.

Suggested fix: Update the session with form data before navigating using mergeSession to persist the data.

2. Type Safety Issue (Medium Priority)

Line 36-39 in FormPreview.tsx spreads routeParams (type RouteData/qs.ParsedQs) into URLSearchParams constructor, which expects Record<string, string>. RouteData can contain nested objects or arrays, causing a type mismatch.

3. No Boundary Checking (Medium Priority)

The navigation logic doesn't check if there's actually a next page to navigate to. This could lead to navigating to non-existent pages. Should check against the total number of pages in the form blueprint.

4. Action Handling Discrepancy (Low Priority)

In Form.tsx:86-116, the non-preview form submission extracts the action from the submit button (submitEvent.submitter). This allows handling both Next and Back buttons. The preview mode doesn't support this, making it impossible to navigate backwards in preview.

5. Missing updatePrompt Call (Medium Priority)

In the non-preview path (Form.tsx:105), updatePrompt(data) is called before onSubmit. The preview path doesn't call this, which might cause validation issues or component state inconsistencies.

Test Coverage

• No tests were added for this functionality
• Recommend adding tests for form submission in preview mode, page navigation with form data preservation, boundary cases (first/last page), and back button functionality if supported

Code Quality

• Code follows existing patterns in the codebase
• TypeScript usage is mostly good, but has the type safety issue mentioned above
• Comments could be more descriptive

Security and Performance

• No security concerns identified
• No performance concerns

Recommendations

Must Address Before Merge:

1. Implement form data persistence in handleSubmit
2. Fix the type safety issue with URLSearchParams

Should Address:
3. Add boundary checking for page navigation
4. Consider supporting back navigation in preview mode
5. Add test coverage

Nice to Have:
6. Add comments explaining the differences between preview and non-preview modes
7. Consider refactoring to reduce duplication between the two form rendering paths

Overall, this is a good start to fixing the preview functionality, but the form data loss issue needs to be addressed before merging to ensure the preview mode accurately simulates the real form experience.

[claude]

Pull Request Review: Add submit (next page) simulation on form preview

Summary

This PR successfully implements form submission and page navigation functionality in the preview mode, fixing issue #26 where the Next button wasn't working. The implementation follows a clean approach by adding a submit handler to FormPreview and ensuring proper form state management.

✅ Strengths

1. Clean separation of concerns: The preview-specific submission logic is properly isolated in FormPreview.tsx, keeping the Form component flexible for both preview and production modes.

2. Proper form state management: The addition of formMethods.reset() when the page changes prevents form data from bleeding across pages, which is critical for multi-page forms.

3. Consistent with existing patterns: The use of applyPromptResponse follows the established pattern seen in the non-preview Form component.

4. Good error handling: Console warnings are logged when applyPromptResponse fails, which aids debugging.

🔍 Potential Issues & Concerns

1. Missing Dependency in useEffect (Form.tsx:83-87)

The dependency array is missing formMethods, which could cause stale references. Since useForm typically returns a stable reference, this might not be a practical issue, but consider adding an ESLint disable comment with explanation if intentional.

2. Error Handling Inconsistency (FormPreview.tsx:39-42)

Looking at the source code (response.ts:21-32), applyPromptResponse always returns success: true - it never fails. This makes the error check dead code. Either remove the error check since it's unreachable, or if future error handling is anticipated, consider how to communicate validation errors to the user.

3. No Bounds Checking for Page Navigation (FormPreview.tsx:48-54)

CRITICAL: The code increments the page number without checking if there's actually a next page. This could navigate to non-existent pages. Add validation to prevent navigating beyond the last page.

4. URLSearchParams Spread Operation (FormPreview.tsx:50-52)

Spreading routeParams into URLSearchParams might not work as expected if routeParams isn't a plain object. Verify that routeParams is a plain object or use URLSearchParams methods directly.

5. Missing Dependencies in useEffect (FormPreview.tsx:19-30)

The useEffect dependency array is missing session, routeParams, and setSession. Add missing dependencies or use a ref pattern to avoid stale closures.

[claude]

🧪 Testing Concerns

No tests were added for the new functionality. Recommendations:

1. Add unit tests for the handleSubmit function in FormPreview
2. Add integration tests for page navigation
3. Test form state reset behavior when navigating between pages

Given the project uses Vitest and has Storybook integration, consider adding component tests using @vitest/browser.

🎨 Code Quality & Style

1. Fragment Removal: Good cleanup removing the unnecessary React fragment in FormContents ✅
2. Consistent Formatting: Code follows project conventions ✅
3. Comments: The comment explaining the form reset is clear and helpful ✅

🔒 Security & Performance

• Security: No significant security issues identified
• Performance: The formMethods.reset() on every page change is appropriate. Consider memoizing the handleSubmit callback if FormPreview re-renders frequently.

📝 Recommendations Summary

High Priority:

1. Add bounds checking for page navigation
2. Fix useEffect dependency arrays or document why they're intentional
3. Add tests for the new functionality

Medium Priority:

1. Remove unreachable error handling or implement proper error UI
2. Verify URLSearchParams spread behavior
3. Consider memoizing handleSubmit

✨ Overall Assessment

This is a solid implementation that successfully fixes the reported issue. The code is clean and follows existing patterns. With the recommended fixes for edge cases and the addition of tests, this will be production-ready.

Suggested Action: Address high-priority items before merging, particularly page bounds checking and dependency array issues.

[claude]

Code Review for PR #28: Add submit (next page) simulation on form preview

Summary

This PR fixes issue #26 by enabling the "Next" button functionality in the form preview mode. The implementation adds form submission handling and page navigation in the preview context.

---

✅ What Works Well

1. Clear problem/solution: The PR addresses a specific UX gap where the next button was non-functional in preview mode
2. Proper state management: Uses existing applyPromptResponse API to validate and apply form data before navigation
3. Form reset on navigation: Prevents field value bleed between pages (lines 81-89 in Form.tsx)
4. Error handling: Includes console warning when validation fails (FormPreview.tsx:40)

---

🔍 Potential Issues & Concerns

1. Missing dependency in useEffect (Form.tsx:85-89)

useEffect(() => {
  if (isPreview && hasOnSubmit) {
    formMethods.reset({});
  }
}, [session.route?.params.page, isPreview, hasOnSubmit]);

Issue: Missing formMethods in dependency array. React Hook useEffect has a missing dependency.

Impact: May cause stale closures or unexpected behavior if formMethods reference changes.

Recommendation: Either add formMethods to dependencies, or use formMethods.reset directly with a ref if reset function is stable.

---

2. Type safety concern with newParams (FormPreview.tsx:50-53)

const newParams = new URLSearchParams({
  ...routeParams,
  page: nextPage.toString(),
});

Issue: routeParams is typed as RouteData which is qs.ParsedQs - a complex nested type that may contain arrays, objects, or undefined values. Spreading this directly into URLSearchParams constructor (which expects Record<string, string>) may cause runtime errors.

Impact: Could fail at runtime if routeParams contains nested objects or arrays.

Recommendation:

const newParams = new URLSearchParams({
  ...Object.fromEntries(
    Object.entries(routeParams).map(([k, v]) => [k, String(v)])
  ),
  page: nextPage.toString(),
});

---

3. No boundary check for page navigation (FormPreview.tsx:47-49)

const currentPage = Number(routeParams.page) || 0;
const nextPage = currentPage + 1;

Issue: No validation that nextPage is within valid bounds. Users could navigate beyond the last page.

Impact: Could result in blank pages or errors when navigating past the final page.

Recommendation: Add validation against the total number of pages in the session/blueprint before navigating.

---

4. Conditional rendering complexity (Form.tsx:96-144)

The form now has three rendering paths:

• !isPreview (lines 96-126)
• isPreview && onSubmit (lines 127-139)
• !(isPreview && onSubmit) (lines 140-143)

Issue: The logic is becoming complex with nested ternaries. The middle branch (preview with submit) duplicates much of the non-preview form structure.

Impact: Harder to maintain, potential for bugs if one branch is updated without the others.

Recommendation: Consider extracting the form rendering logic into a helper component or refactor to reduce duplication:

const FormWrapper = isPreview && !onSubmit ? 'div' : 'form';
const formProps = (isPreview && !onSubmit) ? {} : { /* form-specific props */ };

---

5. Missing validation error display

Issue: When applyPromptResponse returns success: false, the error is only logged to console. Users get no visual feedback.

Impact: Poor UX - users won't know why the form didn't advance.

Recommendation: Display validation errors in the UI, possibly using the existing error state mechanism.

---

🧪 Test Coverage

Concern: No test files found for these components.

Recommendation: Add tests covering:

• Form reset on page navigation
• Submit handler validation success/failure paths
• Page boundary conditions
• RouteParams type handling edge cases

---

🎯 Performance Considerations

Minor: The useEffect that resets the form (Form.tsx:85) will run on every render where dependencies change. This should be fine for typical usage but consider using useCallback for hasOnSubmit if performance issues arise.

---

🔒 Security

No security concerns identified. The code properly validates input through applyPromptResponse before updating state.

---

📝 Suggestions

1. Add JSDoc comments for the new handleSubmit function explaining its purpose and parameters
2. Consider extracting magic strings: 'submit' action type appears in multiple places - consider a constant
3. Error handling: Consider adding error boundaries around navigation logic

---

Verdict

The PR solves the immediate problem effectively but has several issues that should be addressed:

Must fix before merge:

• Type safety issue with URLSearchParams construction (Flexion forms release #2)
• Missing useEffect dependency (Deploy to demo #1)

Should fix:

• Page boundary validation (Changesets publish release #3)
• User-facing error messages (Version Packages #5)

Nice to have:

• Test coverage
• Refactor conditional rendering complexity (Remove tests from precommit hook #4)

Overall, the approach is sound and follows existing patterns in the codebase. With the critical fixes applied, this will be a solid addition.